Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/aws-cdk-lib/aws-ecr/lib/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ export abstract class RepositoryBase extends Resource implements IRepository {
}
const repoAndPrincipalAccountCompare = Token.compareStrings(this.env.account, principalAccount);
if (repoAndPrincipalAccountCompare === TokenComparison.BOTH_UNRESOLVED ||
repoAndPrincipalAccountCompare === TokenComparison.SAME) {
repoAndPrincipalAccountCompare === TokenComparison.SAME) {
return undefined;
}

Expand Down Expand Up @@ -657,7 +657,7 @@ export class Repository extends RepositoryBase {
throw new UnscopedValidationError('Could not determine repository identifier from provided options.');
}

const response: {[key: string]: any}[] = ContextProvider.getValue(scope, {
const response: { [key: string]: any }[] = ContextProvider.getValue(scope, {
provider: cxschema.ContextProvider.CC_API_PROVIDER,
props: {
typeName: 'AWS::ECR::Repository',
Expand Down Expand Up @@ -840,7 +840,9 @@ export class Repository extends RepositoryBase {
this.enableAutoDeleteImages();
}

this.node.addValidation({ validate: () => this.policyDocument?.validateForResourcePolicy() ?? [] });
this.node.addValidation({
validate: () => this.policyDocument?.validateForResourcePolicy({ skipResourceValidation: true }) ?? [],
});
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-iam/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -960,4 +960,4 @@ const instanceProfile = iam.InstanceProfile.fromInstanceProfileAttributes(this,
* Policy names are not required - the CDK logical ID will be used and ensured to be unique.
* Policies are validated during synthesis to ensure that they have actions, and that policies
attached to IAM principals specify relevant resources, while policies attached to resources
specify which IAM principals they apply to.
specify both which IAM principals they apply to and which resources they govern.
25 changes: 22 additions & 3 deletions packages/aws-cdk-lib/aws-iam/lib/policy-document.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { IConstruct } from 'constructs';
import { PolicyStatement, deriveEstimateSizeOptions } from './policy-statement';
import { PolicyStatement, deriveEstimateSizeOptions, ResourcePolicyValidationOptions } from './policy-statement';
import { mergeStatements } from './private/merge-statements';
import { PostProcessPolicyDocument } from './private/postprocess-policy-document';
import * as cdk from '../../core';
Expand Down Expand Up @@ -156,12 +156,31 @@ export class PolicyDocument implements cdk.IResolvable {
*
* @see https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html#access_policies-json
*
* @param options Optional validation options
* @returns An array of validation error messages, or an empty array if the document is valid.
*/
public validateForResourcePolicy(): string[] {
public validateForResourcePolicy(options?: ResourcePolicyValidationOptions): string[] {
const errors = new Array<string>();
for (const statement of this.statements) {
errors.push(...statement.validateForResourcePolicy());
errors.push(...statement.validateForResourcePolicy(options));
}
return errors;
}

/**
* Validate that all policy statements in the policy document satisfies the
* requirements for a trust policy (assume role policy).
*
* Trust policies are a special type of resource-based policy where the resource is implicit (the role itself).
*
* @see https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html
*
* @returns An array of validation error messages, or an empty array if the document is valid.
*/
public validateForTrustPolicy(): string[] {
const errors = new Array<string>();
for (const statement of this.statements) {
errors.push(...statement.validateForTrustPolicy());
}
return errors;
}
Expand Down
46 changes: 40 additions & 6 deletions packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,19 @@ import { LITERAL_STRING_KEY, mergePrincipal, sum } from './private/util';
import * as cdk from '../../core';
import { UnscopedValidationError } from '../../core';

/**
* Options for resource-based policy validation
*/
export interface ResourcePolicyValidationOptions {
/**
* Whether to skip resource validation for policies where resources are implicit
* (e.g., ECR repository policies where the resource is the repository itself)
*
* @default false
*/
readonly skipResourceValidation?: boolean;
}

const ensureArrayOrUndefined = (field: any) => {
if (field === undefined) {
return undefined;
Expand Down Expand Up @@ -80,7 +93,7 @@ export class PolicyStatement {
private readonly _notPrincipal: { [key: string]: any[] } = {};
private readonly _resource = new OrderedSet<string>();
private readonly _notResource = new OrderedSet<string>();
private readonly _condition: { [key: string]: any } = { };
private readonly _condition: { [key: string]: any } = {};
private _sid?: string;
private _effect: Effect;
private principalConditionsJson?: string;
Expand Down Expand Up @@ -540,13 +553,34 @@ export class PolicyStatement {
/**
* Validate that the policy statement satisfies all requirements for a resource-based policy.
*
* @param options Optional validation options
* @returns An array of validation error messages, or an empty array if the statement is valid.
*/
public validateForResourcePolicy(): string[] {
public validateForResourcePolicy(options?: ResourcePolicyValidationOptions): string[] {
const errors = this.validateForAnyPolicy();
if (this._principals.length === 0 && this._notPrincipals.length === 0) {
errors.push('A PolicyStatement used in a resource-based policy must specify at least one IAM principal.');
}
// Only validate resources if not explicitly skipped (for services like ECR where resources are implicit)
if (!options?.skipResourceValidation && this._resource.length === 0 && this._notResource.length === 0) {
errors.push('A PolicyStatement used in a resource-based policy must specify at least one resource.');
}
return errors;
}

/**
* Validate that the policy statement satisfies all requirements for a trust policy (assume role policy).
*
* Trust policies are a special type of resource-based policy where the resource is implicit (the role itself).
*
* @returns An array of validation error messages, or an empty array if the statement is valid.
*/
public validateForTrustPolicy(): string[] {
const errors = this.validateForAnyPolicy();
if (this._principals.length === 0 && this._notPrincipals.length === 0) {
errors.push('A PolicyStatement used in a trust policy must specify at least one IAM principal.');
}
// Note: Trust policies do not require resources because the resource is implicit (the role itself)
return errors;
}

Expand Down Expand Up @@ -789,7 +823,7 @@ export interface PolicyStatementProps {
*
* @default - no condition
*/
readonly conditions?: {[key: string]: any};
readonly conditions?: { [key: string]: any };

/**
* Whether to allow or deny the actions in this statement
Expand All @@ -802,15 +836,15 @@ export interface PolicyStatementProps {
class JsonPrincipal extends PrincipalBase {
public readonly policyFragment: PrincipalPolicyFragment;

constructor(json: any = { }) {
constructor(json: any = {}) {
super();

// special case: if principal is a string, turn it into a "LiteralString" principal,
// so we render the exact same string back out.
if (typeof(json) === 'string') {
if (typeof (json) === 'string') {
json = { [LITERAL_STRING_KEY]: [json] };
}
if (typeof(json) !== 'object') {
if (typeof (json) !== 'object') {
throw new UnscopedValidationError(`JSON IAM principal should be an object, got ${JSON.stringify(json)}`);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,7 @@ export class Role extends Resource implements IRole {

private validateRole(): string[] {
const errors = new Array<string>();
errors.push(...this.assumeRolePolicy?.validateForResourcePolicy() ?? []);
errors.push(...this.assumeRolePolicy?.validateForTrustPolicy() ?? []);
for (const policy of Object.values(this.inlinePolicies)) {
errors.push(...policy.validateForIdentityPolicy());
}
Expand Down
128 changes: 115 additions & 13 deletions packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,23 @@ describe('IAM policy document', () => {

expect(stack.resolve(p.toStatementJson())).toEqual({
Action:
['sqs:SendMessage',
'dynamodb:CreateTable',
'dynamodb:DeleteTable'],
['sqs:SendMessage',
'dynamodb:CreateTable',
'dynamodb:DeleteTable'],
Resource: ['myQueue', 'yourQueue', '*'],
Effect: 'Allow',
Principal:
{
AWS:
{
'Fn::Join':
['',
['arn:',
{ Ref: 'AWS::Partition' },
':iam::my',
{ account: 'account' },
'name:root']],
},
{
'Fn::Join':
['',
['arn:',
{ Ref: 'AWS::Partition' },
':iam::my',
{ account: 'account' },
'name:root']],
},
},
Condition: { StringEquals: { 'sts:ExternalId': '12221121221' } },
});
Expand Down Expand Up @@ -852,7 +852,10 @@ describe('IAM policy document', () => {
// THEN
const validationErrorsForResourcePolicy: string[] = policyStatement.validateForResourcePolicy();
// const validationErrorsForIdentityPolicy: string[] = policyStatement.validateForIdentityPolicy();
expect(validationErrorsForResourcePolicy).toEqual(['A PolicyStatement must specify at least one \'action\' or \'notAction\'.']);
expect(validationErrorsForResourcePolicy).toEqual([
'A PolicyStatement must specify at least one \'action\' or \'notAction\'.',
'A PolicyStatement used in a resource-based policy must specify at least one resource.',
]);
});

test('validation error if policy statement for resource-based policy has no principals specified', () => {
Expand All @@ -862,6 +865,105 @@ describe('IAM policy document', () => {

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy();
expect(validationErrors).toEqual([
'A PolicyStatement used in a resource-based policy must specify at least one IAM principal.',
'A PolicyStatement used in a resource-based policy must specify at least one resource.',
]);
});

test('validation error if policy statement for resource-based policy has no resources specified', () => {
const policyStatement = new PolicyStatement({
actions: ['s3:GetObject'],
principals: [new AnyPrincipal()],
// Missing: resources
});

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy();
expect(validationErrors).toEqual(['A PolicyStatement used in a resource-based policy must specify at least one resource.']);
});

test('validation passes when both principals and resources are specified', () => {
const policyStatement = new PolicyStatement({
actions: ['s3:GetObject'],
principals: [new AnyPrincipal()],
resources: ['arn:aws:s3:::my-bucket/*'],
});

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy();
expect(validationErrors).toEqual([]);
});

test('validation passes with NotResource instead of Resource', () => {
const policyStatement = new PolicyStatement({
actions: ['s3:GetObject'],
principals: [new AnyPrincipal()],
notResources: ['arn:aws:s3:::excluded-bucket/*'],
});

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy();
expect(validationErrors).toEqual([]);
});

test('validation error if policy statement for resource-based policy has neither principals nor resources', () => {
const policyStatement = new PolicyStatement({
actions: ['s3:GetObject'],
// Missing: both principals and resources
});

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy();
expect(validationErrors).toEqual([
'A PolicyStatement used in a resource-based policy must specify at least one IAM principal.',
'A PolicyStatement used in a resource-based policy must specify at least one resource.',
]);
});

test('validation error if policy statement for trust policy has no principals specified', () => {
const policyStatement = new PolicyStatement({
actions: ['sts:AssumeRole'],
// Missing: principals (resources not needed for trust policies)
});

// THEN
const validationErrors: string[] = policyStatement.validateForTrustPolicy();
expect(validationErrors).toEqual(['A PolicyStatement used in a trust policy must specify at least one IAM principal.']);
});

test('validation passes for trust policy with principals but no resources', () => {
const policyStatement = new PolicyStatement({
actions: ['sts:AssumeRole'],
principals: [new AnyPrincipal()],
// No resources needed for trust policies
});

// THEN
const validationErrors: string[] = policyStatement.validateForTrustPolicy();
expect(validationErrors).toEqual([]);
});

test('validation passes for resource-based policy with skipResourceValidation option', () => {
const policyStatement = new PolicyStatement({
actions: ['ecr:GetDownloadUrlForLayer'],
principals: [new AnyPrincipal()],
// Missing: resources (but validation is skipped)
});

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy({ skipResourceValidation: true });
expect(validationErrors).toEqual([]);
});

test('validation still requires principals even with skipResourceValidation', () => {
const policyStatement = new PolicyStatement({
actions: ['ecr:GetDownloadUrlForLayer'],
// Missing: both principals and resources
});

// THEN
const validationErrors: string[] = policyStatement.validateForResourcePolicy({ skipResourceValidation: true });
expect(validationErrors).toEqual(['A PolicyStatement used in a resource-based policy must specify at least one IAM principal.']);
});
});
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-iam/test/role.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,7 @@ describe('IAM role', () => {
});
role.assumeRolePolicy?.addStatements(new PolicyStatement({ actions: ['*'] }));

expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
expect(() => app.synth()).toThrow(/A PolicyStatement used in a trust policy must specify at least one IAM principal/);
});
});

Expand Down
13 changes: 13 additions & 0 deletions packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,19 @@ describe('bucket policy', () => {
expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
});

test('fails if bucket policy has no resources', () => {
const app = new App();
const stack = new Stack(app, 'my-stack');
const myBucket = new s3.Bucket(stack, 'MyBucket');
myBucket.addToResourcePolicy(new PolicyStatement({
actions: ['s3:GetObject*'],
principals: [new AnyPrincipal()],
// Missing: resources
}));

expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one resource/);
});

describe('fromCfnBucketPolicy()', () => {
const stack = new Stack();

Expand Down
2 changes: 2 additions & 0 deletions packages/aws-cdk-lib/aws-sns/test/sns.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -486,10 +486,12 @@ describe('Topic', () => {
topic.addToResourcePolicy(new iam.PolicyStatement({
actions: ['service:statement0'],
principals: [new iam.ArnPrincipal('arn')],
resources: ['*'],
}));
topic.addToResourcePolicy(new iam.PolicyStatement({
actions: ['service:statement1'],
principals: [new iam.ArnPrincipal('arn')],
resources: ['*'],
}));

Template.fromStack(stack).hasResourceProperties('AWS::SNS::TopicPolicy', {
Expand Down
Loading