Skip to content

Conversation

@pahud
Copy link
Contributor

@pahud pahud commented Dec 3, 2025

Issue # (if applicable)

N/A - Security improvement and test reliability fix. Related to #36224

Reason for this change

Primary goal: Current implementation for anonymousAccess: false is essentially derived from EFS console. With this default resource policy

  1. No identity principal can mount this filesystem until explicitly granted with ClientMount
  2. No cross-account identity can do that until we add an additional statement in the policy to allow that with ClientMount

so this policy is general fine with protective purpose but confusing. We should scope down the resource policy principal definition from any arn('*') to account root principal as this could be a concern(though it's default behavior from EFS console)

image

Secondary goal: Update the existing integration test to validate this policy change. The current test uses EC2 NANO instances with AL2023, which has critical reliability issues:

  • NANO instances are subject to OOM (Out of Memory) kills and cannot successfully complete the test
  • AL2023's automatic dnf package updates conflict with our NFS utils package installation, causing timeouts
  • This combination makes the test extremely difficult to maintain and cannot guarantee successful deployment

We replace the EC2-based implementation with Lambda functions to simplify the process and ensure reliable test execution.

Description of changes

1. EFS File System Policy Security Fix (packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts):

  • Changed principal from AnyPrincipal (AWS: '*') to AccountRootPrincipal to scope down overly permissive access
  • Resource policy intentionally does NOT include ClientMount action - principals must be explicitly granted via grantRead(), grantReadWrite(), or grantRootAccess() methods
  • Added comprehensive documentation explaining the policy behavior and why ClientMount is excluded
  • Restricts access to IAM principals within the same AWS account only
  • Prevents unintended cross-account access while requiring explicit grants for same-account IAM principals

2. Integration Test Refactor (packages/@aws-cdk-testing/framework-integ/test/aws-efs/test/integ.efs.permission.ts):

  • Replaced EC2 NANO instances with Lambda functions to eliminate OOM and package management issues
  • Added EFS Access Point for Lambda mounting at /mnt/efs
  • Replaced SSM command invocations with direct Lambda function invocations
  • Added ASCII architecture diagram showing Lambda-to-EFS connectivity
  • Three Lambda functions test different scenarios: read-write, read-only, no permissions
  • Eliminates dnf auto-update conflicts and ensures reliable test execution

Key technical changes:

  • Policy: AnyPrincipal()AccountRootPrincipal(), ClientMount intentionally excluded from resource policy
  • Test: EC2 NANO + SSM → Lambda + direct invocations
  • Reliability: Eliminated OOM kills, package management conflicts, and deployment failures

Describe any new or updated permissions being added

EFS File System Policy Change:

  • Before: Used AWS: '*' (any principal) with ClientWrite and ClientRootAccess actions
  • After: Uses AccountRootPrincipal (account root) with only ClientWrite and ClientRootAccess actions

This change is more restrictive and follows AWS security best practices:

  • Primary improvement: Scopes down from AnyPrincipal to AccountRootPrincipal, limiting access to IAM principals within the same AWS account
  • Prevents anonymous cross-account access
  • Requires explicit grants via grantRead(), grantReadWrite(), or grantRootAccess() for principals to mount the filesystem
  • ClientMount is intentionally excluded from the resource policy to enforce explicit grants through identity-based permissions

Description of how you validated changes

  • Unit tests: Updated 5 test cases in efs-file-system.test.ts to reflect the new policy structure with AccountRootPrincipal and ClientMount action
  • Integration tests: The refactored integration test validates:
    • WriteLambda can write and read files on EFS (grantReadWrite)
    • ReadLambda can read but not write (grantRead, permission denied on write)
    • AnonymousLambda cannot access EFS (no grant, access denied)
    • All tests use Lambda function invocations with payload assertions

Checklist


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

- Replace AnyPrincipal with AccountRootPrincipal in EFS resource policy to block anonymous NFS access
- Add ClientMount action to the policy statement alongside existing Write and RootAccess actions
- Update policy to enforce IAM authentication by switching EFS from default anonymous access mode to IAM enforcement mode
- Add comprehensive documentation explaining why AccountRootPrincipal is used instead of AnyPrincipal
- Update all related test cases to reflect the new AccountRootPrincipal ARN in policy assertions
- Ensure same-account IAM principals can still access the file system with proper identity-based permissions while preventing cross-account anonymous access
@aws-cdk-automation aws-cdk-automation requested a review from a team December 3, 2025 18:23
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Dec 3, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 3, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@aws-cdk-automation aws-cdk-automation added the pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes. label Dec 3, 2025
- Update EFS filesystem one-zone integration test snapshot manifests
- Update EFS filesystem policy integration test snapshot files
- Update EFS filesystem protection integration test snapshots
- Update EFS filesystem replication integration test snapshots
- Update EFS from imported subnet integration test snapshots
- Update EFS transition integration test snapshots
- Update EFS permission integration test snapshots and source
- Regenerate CloudFormation templates and asset hashes
- Simplify analytics construct data in snapshot manifests
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 4, 2025 16:04

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

- Update manifest.json files across all EFS integration test snapshots
- Regenerate CloudFormation template asset URLs for all test cases
- Update tree.json files to reflect current construct tree state
- Refresh assets.json files with latest asset configurations
- Update EFS filesystem implementation to reflect snapshot changes
- Regenerate test snapshots for efs-filesystem-one-zone integration test
- Regenerate test snapshots for efs-filesystem-policy integration test
- Regenerate test snapshots for efs-filesystem-protection integration test
- Regenerate test snapshots for efs-filesystem-replication integration test
- Regenerate test snapshots for efs-from-imported-subnet integration test
- Regenerate test snapshots for efs-transition integration test
- Regenerate test snapshots for efs integration test
- Regenerate test snapshots for efs.permission integration test
@pahud pahud changed the title fix(efs): add missing ClientMount action to file system policy when blocking anonymous access chore(efs): scope down overly permissive AnyPrincipal in anonymous access policy Dec 4, 2025
@pahud pahud added p2 and removed p1 labels Dec 4, 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-further-review PR requires additional review from our team specialists due to the scope or complexity of changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants