Skip to content

Conversation

@pahud
Copy link
Contributor

@pahud pahud commented Aug 21, 2025

Issue # (if applicable)

Closes #35296.

Reason for this change

CDK currently allows creating resource-based policies (such as S3 bucket policies, SNS topic policies, KMS key policies) with PolicyStatements that lack the required resources property. This causes deployment-time CloudFormation failures instead of providing immediate synthesis-time validation errors.

This creates a poor developer experience where:

  • Developers waste time on failed deployments that could be caught earlier
  • Error messages come from CloudFormation rather than CDK with less actionable guidance
  • The feedback loop is much slower (deployment time vs synthesis time)

The root cause is that PolicyStatement.validateForResourcePolicy() validates principals but not resources, despite both being required for resource-based policies according to AWS IAM documentation.

Description of changes

This change enhances the existing validation infrastructure to catch missing resources at synthesis time while handling the nuanced differences between different types of resource-based policies:

Enhanced PolicyStatement.validateForResourcePolicy():

  • Added resource validation alongside existing principal validation to ensure both principals AND resources are specified for resource-based policies
  • Added optional ResourcePolicyValidationOptions parameter with JSII-compatible interface for services with implicit resource contexts

New ResourcePolicyValidationOptions interface:

  • Created proper TypeScript interface with skipResourceValidation?: boolean property to handle services like ECR where resources are implicit
  • Ensures cross-language compatibility via JSII

New PolicyStatement.validateForTrustPolicy():

  • Created dedicated validation method for IAM Role trust policies that require principals but not resources (since the resource is implicit - the role itself)

Updated PolicyDocument.validateForTrustPolicy():

  • Added corresponding document-level validation method for trust policies with the same options interface

Updated Role validation:

  • Modified IAM Role validation to use the new trust policy validation instead of incorrectly using resource policy validation

ECR Repository policy handling:

  • Updated ECR repository validation to use { skipResourceValidation: true } since ECR repository policies have implicit resources (the repository itself)

Cross-service impact:
This change automatically benefits 8+ AWS services that use resource-based policies:

  • S3 (Bucket policies) - requires explicit resources, uses default validation
  • SNS (Topic policies) - requires explicit resources, uses default validation
  • KMS (Key policies) - requires explicit resources, uses default validation
  • SQS (Queue policies) - requires explicit resources, uses default validation
  • ECR (Repository policies) - uses implicit resources with skipResourceValidation: true
  • Secrets Manager (Secret policies) - requires explicit resources, uses default validation
  • Kinesis (Stream policies) - requires explicit resources, uses default validation
  • And others using the same validation infrastructure

API Changes:

New Interface:

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;
}

Updated Method Signatures:

// PolicyStatement
public validateForResourcePolicy(options?: ResourcePolicyValidationOptions): string[]
public validateForTrustPolicy(): string[]

// PolicyDocument 
public validateForResourcePolicy(options?: ResourcePolicyValidationOptions): string[]
public validateForTrustPolicy(): string[]

Describe any new or updated permissions being added

N/A - This change only adds synthesis-time validation logic and does not modify IAM permissions, resource access patterns, or CloudFormation template generation.

Description of how you validated changes

Unit tests:
Added 7 comprehensive unit tests in packages/aws-cdk-lib/aws-iam/test/policy-document.test.ts:

  • validation error if policy statement for resource-based policy has no resources specified
  • validation passes when both principals and resources are specified
  • validation passes with NotResource instead of Resource
  • validation error if policy statement for trust policy has no principals specified
  • validation passes for trust policy with principals but no resources
  • validation passes for resource-based policy with skipResourceValidation option
  • validation still requires principals even with skipResourceValidation

Integration tests:
Added integration test in packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts:

  • fails if bucket policy has no resources - demonstrates end-to-end validation from S3 bucket policy creation through CDK synthesis

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…ed policies

- Add resource validation to PolicyStatement.validateForResourcePolicy() method
- Create new validateForTrustPolicy() method for IAM Role trust policies
- Update Role validation to use trust policy validation instead of resource policy validation
- Add comprehensive unit tests covering all validation scenarios
- Add S3 bucket policy integration test demonstrating end-to-end validation
- Automatically benefits 8+ AWS services (S3, SNS, KMS, SQS, ECR, Secrets Manager, Kinesis)

Closes aws#35296

This change provides synthesis-time validation for missing resources in resource-based policies,
replacing deployment-time CloudFormation errors with immediate, actionable CDK synthesis errors.
No breaking changes - only catches currently invalid code that fails at deployment.
@aws-cdk-automation aws-cdk-automation requested a review from a team August 21, 2025 19:51
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Aug 21, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 21, 2025
@pahud pahud marked this pull request as ready for review August 23, 2025 12:22
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws_s3: Missing validation for PolicyStatement.Resources inside the construct BucketPolicy

3 participants