diff --git a/packages/aws-cdk-lib/aws-ecr/lib/repository.ts b/packages/aws-cdk-lib/aws-ecr/lib/repository.ts index 9424ab9aae637..67cae2b2c1c39 100644 --- a/packages/aws-cdk-lib/aws-ecr/lib/repository.ts +++ b/packages/aws-cdk-lib/aws-ecr/lib/repository.ts @@ -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; } @@ -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', @@ -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 }) ?? [], + }); } /** diff --git a/packages/aws-cdk-lib/aws-iam/README.md b/packages/aws-cdk-lib/aws-iam/README.md index 1aee02ab60425..b80816c9edc2a 100644 --- a/packages/aws-cdk-lib/aws-iam/README.md +++ b/packages/aws-cdk-lib/aws-iam/README.md @@ -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. diff --git a/packages/aws-cdk-lib/aws-iam/lib/policy-document.ts b/packages/aws-cdk-lib/aws-iam/lib/policy-document.ts index 663a0e083b462..f87adb551d4dd 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/policy-document.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/policy-document.ts @@ -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'; @@ -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(); 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(); + for (const statement of this.statements) { + errors.push(...statement.validateForTrustPolicy()); } return errors; } diff --git a/packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts b/packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts index 5ee99ed959097..af7fb32c24bb0 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/policy-statement.ts @@ -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; @@ -80,7 +93,7 @@ export class PolicyStatement { private readonly _notPrincipal: { [key: string]: any[] } = {}; private readonly _resource = new OrderedSet(); private readonly _notResource = new OrderedSet(); - private readonly _condition: { [key: string]: any } = { }; + private readonly _condition: { [key: string]: any } = {}; private _sid?: string; private _effect: Effect; private principalConditionsJson?: string; @@ -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; } @@ -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 @@ -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)}`); } diff --git a/packages/aws-cdk-lib/aws-iam/lib/role.ts b/packages/aws-cdk-lib/aws-iam/lib/role.ts index a036dd24900dc..85e5b4da2f887 100644 --- a/packages/aws-cdk-lib/aws-iam/lib/role.ts +++ b/packages/aws-cdk-lib/aws-iam/lib/role.ts @@ -719,7 +719,7 @@ export class Role extends Resource implements IRole { private validateRole(): string[] { const errors = new Array(); - errors.push(...this.assumeRolePolicy?.validateForResourcePolicy() ?? []); + errors.push(...this.assumeRolePolicy?.validateForTrustPolicy() ?? []); for (const policy of Object.values(this.inlinePolicies)) { errors.push(...policy.validateForIdentityPolicy()); } diff --git a/packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts b/packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts index 42665ab260876..97adf5457ea61 100644 --- a/packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts +++ b/packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts @@ -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' } }, }); @@ -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', () => { @@ -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.']); }); }); diff --git a/packages/aws-cdk-lib/aws-iam/test/role.test.ts b/packages/aws-cdk-lib/aws-iam/test/role.test.ts index 5069ad77312d4..7ee5d453e792a 100644 --- a/packages/aws-cdk-lib/aws-iam/test/role.test.ts +++ b/packages/aws-cdk-lib/aws-iam/test/role.test.ts @@ -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/); }); }); diff --git a/packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts b/packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts index 94a26e24e8995..2b97ce53fb1ac 100644 --- a/packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts +++ b/packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts @@ -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(); diff --git a/packages/aws-cdk-lib/aws-sns/test/sns.test.ts b/packages/aws-cdk-lib/aws-sns/test/sns.test.ts index 901a21c3bd6ba..270d9ad4f9f8a 100644 --- a/packages/aws-cdk-lib/aws-sns/test/sns.test.ts +++ b/packages/aws-cdk-lib/aws-sns/test/sns.test.ts @@ -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', {