feat(efs): add grantRead and grantReadWrite, grantRootAccess to FileSystem by WinterYukky · Pull Request #25486 · aws/aws-cdk

WinterYukky · 2023-05-08T17:37:51Z

What change

I added grantRead() and grantReadWrite(), grantRootAccess() to efs.FileSystem as Beta1 method.

Why need this change?

To make IAM authentication easier for clients.

Currently, v2.78.0 has implemented grant() method in efs.FileSystem. However, EFS can't restrict only granted client even when customers only use the grant() method. Because EFS default file system policy grants full access to any anonymous client that can connect to the file system using a mount target. To avoid this issue, customers must set file system policies that not grant anonymous clients, to EFS. In this PR, when using the grantXxx method that allows IAM authentication for clients, a file system policy that does not allow anonymous clients is set to efs.FileSystem by default to suit the customer's use case. Next example is grant read and write access to EC2 Instance.

declare const client: ec2.Instance;
const fileSystem = new efs.FileSystem(this, 'FileSystem', {
  vpc: new ec2.Vpc(this, 'VPC'),
});
fileSystem.grantReadWrite(client);

How do I continue to allow anonymous access?

You can use allowAnonymousAccess props for allow anonymous access.

declare const client: ec2.Instance;
const fileSystem = new efs.FileSystem(this, 'FileSystem', {
  vpc: new ec2.Vpc(this, 'VPC'),
  allowAnonymousAccess: true,
});
fileSystem.grantRead(client);

Others

The file system policies created to prevent anonymous clients are based on the AWS Management Console.

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

gitpod-io · 2023-05-08T17:38:02Z

packages/aws-cdk-lib/aws-efs/README.md

WinterYukky · 2023-05-08T17:50:00Z

packages/@aws-cdk-testing/framework-integ/test/aws-efs/test/integ.efs.permission.ts

This integration test tests the following:

Clients granted by grantReadWrite() write files during deployment

Assert that anonymous client mounts fail

Assert that a client authorized by grantRead() will read the file

WinterYukky · 2023-05-08T17:57:12Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+    return iam.Grant.addToPrincipalOrResource({
+      grantee: grantee,
+      actions: actions,
+      resourceArns: [this.fileSystemArn],
+      resource: this,
+    });
+  }


Change the grant function from iam.Grant.addToPrincipal to iam.Grant.addToPrincipalOrResource in grant method, like a same resource that has resource policy.

Examples

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L968-L988

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-sns/lib/topic-base.ts#L134C1-L141

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-kms/lib/key.ts#L164-L196

This reverts commit c600a49.

…Yukky/aws-cdk into feat/efs/add-grant-client

corymhall

Looks great! Especially love the integ test! Just a couple of comments

corymhall · 2023-06-05T11:50:25Z

packages/@aws-cdk-testing/framework-integ/test/aws-efs/test/integ.efs.permission.ts

corymhall · 2023-06-05T12:02:03Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+   * @param conditions The conditions to grant
+   * @internal
+   */
+  public _grantClient(grantee: iam.IGrantable, actions: ClientAction[], conditions?: Record<string, Record<string, unknown>>): iam.Grant {


Does this need to be public or can it be private?

I was planning to use this method on AccessPoint, so I made it public. Currently, it doesn't need to be public, so I will change to private until open the PR that add grant API to AccessPoint :)

corymhall · 2023-06-05T12:03:00Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+   * Grant read permissions for this file system to an IAM principal.
+   * @param grantee The principal to grant read to
+   */
+  public grantReadBeta1(grantee: iam.IGrantable): iam.Grant {


I think it is safe to remove the Beta1 from these. Based on the API I can't
foresee any breaking changes that we would need to make.

corymhall · 2023-06-05T12:03:58Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

-      fileSystemPolicy: Lazy.any({ produce: () => this._fileSystemPolicy }),
+      fileSystemPolicy: Lazy.any({
+        produce: () => {
+          const allowAnonymousAccess = props.allowAnonymousAccess ?? !this._grantedClient;


I think this should be the default behavior. Would you mind/be able to create a
new feature flag and make this the new default?

Does that mean that the behavior of deny anonymous clients should be made the default even when not using grantClient?

yes, but only with the feature flag.

…at/efs/add-grant-client

Pull request has been modified.

…at/efs/add-grant-client

aws-cdk-automation · 2023-07-02T00:10:26Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-07-03T00:09:40Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…Yukky/aws-cdk into feat/efs/add-grant-client

…at/efs/add-grant-client

aws-cdk-automation · 2023-07-24T00:09:13Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-07-25T00:09:00Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…at/efs/add-grant-client

kaizencc

Hi @WinterYukky just a couple minor comments :)

packages/aws-cdk-lib/cx-api/lib/features.ts

packages/aws-cdk-lib/aws-efs/README.md

kaizencc · 2023-08-01T19:42:21Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+      fileSystemPolicy: Lazy.any({
+        produce: () => {
+          const denyAnonymousAccessFlag = FeatureFlags.of(this).isEnabled(cxapi.EFS_DENY_ANONYMOUS_ACCESS) ?? false;
+          const denyAnonymousAccessByDefault = denyAnonymousAccessFlag || this._grantedClient;


help me understand why this._grantedClient is included here?

Originally, my design was to reject anonymous clients only when using grantXXX (e.g. grantRead). Currently, an implementation that rejects anonymous clients by default based on feature flags has been added. Therefore, it is also possible to remove _grantedClient and switch to feature flags only.
However, even if a user who has upgraded the CDK from an older version starts using grantXXX, anonymous clients will not be rejected unless the feature flag is enabled, so I implemented an implementation that determines the permission of an anonymous client with both the feature flag and _grantedClient according to the original purpose. Nonetheless, I'm going to try not to use _grantedClient if it's unnecessary because it affects maintainability. What do you think?

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Pull request has been modified.

aws-cdk-automation · 2023-08-17T00:07:24Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-08-18T00:07:31Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…at/efs/add-grant-client

aws-cdk-automation · 2023-08-23T00:07:32Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

mergify · 2023-08-23T12:45:48Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

mergify · 2023-08-23T15:43:14Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-08-23T16:11:08Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 9101d71
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-08-23T16:11:34Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

feat(efs): add grantXxx

56b9267

aws-cdk-automation requested a review from a team May 8, 2023 17:38

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2 labels May 8, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 8, 2023

WinterYukky commented May 8, 2023

View reviewed changes

WinterYukky added 6 commits May 9, 2023 11:29

Merge branch 'main' into feat/efs/add-grant-client

bc716a1

test(efs): update integ test

c600a49

Revert "test(efs): update integ test"

12178dc

This reverts commit c600a49.

Merge branch 'main' into feat/efs/add-grant-client

481c7e2

test(efs): update integ test stack name

d98bef5

Merge branch 'feat/efs/add-grant-client' of https://github.com/Winter…

3e42209

…Yukky/aws-cdk into feat/efs/add-grant-client

corymhall previously requested changes Jun 5, 2023

View reviewed changes

corymhall self-assigned this Jun 5, 2023

corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 5, 2023

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fe…

96749ca

…at/efs/add-grant-client

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 7, 2023

WinterYukky added 2 commits June 7, 2023 09:31

chore(efs): remove Beta1 from grant APIs

09a8d97

chore(efs): grantClient to private

a727f19

corymhall removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 12, 2023

WinterYukky added 6 commits June 26, 2023 15:39

feat(efs): add @aws-cdk/aws-efs:denyAnonymousAccess feature flag

bb906bb

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fe…

f42c0b8

…at/efs/add-grant-client

chore(efs): update feature flag doc

24791d1

Merge branch 'main' into feat/efs/add-grant-client

722252c

test(efs): update integ

e6a9d69

doc(batch): use efs grant

38b1579

WinterYukky added 3 commits July 20, 2023 01:39

Merge branch 'feat/efs/add-grant-client' of https://github.com/Winter…

32d7c18

…Yukky/aws-cdk into feat/efs/add-grant-client

docs(efs): add iam import

11cf645

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fe…

1629420

…at/efs/add-grant-client

WinterYukky added 2 commits July 25, 2023 02:09

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fe…

515c092

…at/efs/add-grant-client

revert unassociated modules

883994d

kaizencc previously requested changes Aug 1, 2023

View reviewed changes

WinterYukky and others added 2 commits August 7, 2023 23:20

Update packages/aws-cdk-lib/cx-api/lib/features.ts

b3d08e5

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Update packages/aws-cdk-lib/aws-efs/README.md

cb5c97a

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fe…

92d991a

…at/efs/add-grant-client

Merge remote-tracking branch 'origin/main' into pr/WinterYukky/25486

403dafe

rix0rrr assigned rix0rrr and unassigned corymhall Aug 23, 2023

rix0rrr previously approved these changes Aug 23, 2023

View reviewed changes

Merge branch 'main' into feat/efs/add-grant-client

3b64939

rix0rrr approved these changes Aug 23, 2023

View reviewed changes

Merge branch 'main' into feat/efs/add-grant-client

9101d71

mergify bot merged commit 9c12199 into aws:main Aug 23, 2023

peterwoodworth mentioned this pull request Oct 4, 2023

aws_efs (Python): Incorrect File System Policy defaults #27374

Closed

mazyu36 mentioned this pull request Mar 9, 2025

aws-efs: EFS does not contain the mount access policy by default #33669

Closed

1 task

Conversation

WinterYukky commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What change

Why need this change?

How do I continue to allow anonymous access?

Others

Uh oh!

gitpod-io bot commented May 8, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corymhall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jul 2, 2023

Uh oh!

aws-cdk-automation commented Jul 3, 2023

Uh oh!

aws-cdk-automation commented Jul 24, 2023

Uh oh!

aws-cdk-automation commented Jul 25, 2023

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Aug 17, 2023

Uh oh!

aws-cdk-automation commented Aug 18, 2023

Uh oh!

aws-cdk-automation commented Aug 23, 2023

Uh oh!

mergify bot commented Aug 23, 2023

Uh oh!

mergify bot commented Aug 23, 2023

Uh oh!

aws-cdk-automation commented Aug 23, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WinterYukky commented May 8, 2023 •

edited

Loading