-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(efs): add grantRead and grantReadWrite, grantRootAccess to FileSystem #25486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this!
return iam.Grant.addToPrincipalOrResource({ | ||
grantee: grantee, | ||
actions: actions, | ||
resourceArns: [this.fileSystemArn], | ||
resource: this, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the grant function from iam.Grant.addToPrincipal to iam.Grant.addToPrincipalOrResource in grant method, like a same resource that has resource policy.
Examples
This reverts commit c600a49.
…Yukky/aws-cdk into feat/efs/add-grant-client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Especially love the integ test! Just a couple of comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this!
* @param conditions The conditions to grant | ||
* @internal | ||
*/ | ||
public _grantClient(grantee: iam.IGrantable, actions: ClientAction[], conditions?: Record<string, Record<string, unknown>>): iam.Grant { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be public
or can it be private
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
* 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
fileSystemPolicy: Lazy.any({ produce: () => this._fileSystemPolicy }), | ||
fileSystemPolicy: Lazy.any({ | ||
produce: () => { | ||
const allowAnonymousAccess = props.allowAnonymousAccess ?? !this._grantedClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that the behavior of deny anonymous clients should be made the default even when not using grantClient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but only with the feature flag.
…at/efs/add-grant-client
…at/efs/add-grant-client
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. |
1 similar comment
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
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. |
1 similar comment
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @WinterYukky just a couple minor comments :)
fileSystemPolicy: Lazy.any({ | ||
produce: () => { | ||
const denyAnonymousAccessFlag = FeatureFlags.of(this).isEnabled(cxapi.EFS_DENY_ANONYMOUS_ACCESS) ?? false; | ||
const denyAnonymousAccessByDefault = denyAnonymousAccessFlag || this._grantedClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help me understand why this._grantedClient
is included here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
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. |
1 similar comment
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
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. |
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). |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
What change
I added
grantRead()
andgrantReadWrite()
,grantRootAccess()
toefs.FileSystem
as Beta1 method.Why need this change?
To make IAM authentication easier for clients.
Currently, v2.78.0 has implemented
grant()
method inefs.FileSystem
. However, EFS can't restrict only granted client even when customers only use thegrant()
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 thegrantXxx
method that allows IAM authentication for clients, a file system policy that does not allow anonymous clients is set toefs.FileSystem
by default to suit the customer's use case. Next example is grant read and write access to EC2 Instance.How do I continue to allow anonymous access?
You can use
allowAnonymousAccess
props for allow anonymous access.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