-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(iam): managed-policy can again be granted actions on resources #33115
base: main
Are you sure you want to change the base?
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 review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…mment @stylistic/spaced-comment"
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33115 +/- ##
==========================================
- Coverage 81.48% 80.78% -0.70%
==========================================
Files 226 232 +6
Lines 13768 14111 +343
Branches 2416 2453 +37
==========================================
+ Hits 11219 11400 +181
- Misses 2271 2431 +160
- Partials 278 280 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 review is outdated)
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.
The pull request linter fails with the following errors:
❌ CodeCov is indicating a drop in code coverage
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed, add Clarification Request
to a comment.
Dismissing outdated PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -336,7 +336,8 @@ class ManagedPolicyGrantPrincipal implements IPrincipal { | |||
// This property is referenced to add policy statements as a resource-based policy. | |||
// We should fail because a managed policy cannot be used as a principal of a policy document. | |||
// cf. https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#Principal_specifying | |||
throw new Error(`Cannot use a ManagedPolicy '${this._managedPolicy.node.path}' as the 'Principal' or 'NotPrincipal' in an IAM Policy`); | |||
// I32795: Restoring a previous feature where a grant for a managed policy would generate policy document with actions as a CDK feature. This managed policy needs to be attached to a role or principal for it to be used meaningfully. | |||
return new PrincipalPolicyFragment({}); |
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.
The reason tableV2.grantX(someManagedPolicy)
started throwing error is due to this line:
return Grant.addToPrincipalOrResource({ |
Down the call path of tableV2.grantX(someManagedPolicy)
, addToPrincipalOrResource
gets called at some point and it does the following:
- It adds permission to the principal (in this case, it adds permission to
someManagedPolicy
). - It checks if the principal and resource are in different account. If so, it updates the resource based policy to allow the principal to access the resource (in this case, it would try to add the
someManagedPolicy
principal to thetableV2
resource based policy.)
In step 2, when CDK tries to assemble the correct resource based policy, that is where the error is thrown. The intention of the code seems to be that, a Policy or Managed Policy is not a principal, hence throw an error (in other words, it is not possible to allow a Policy to access a resource. You can only allow an identity, aka principal, to access a resource). Logically, this makes sense. It is also explained in the PR that added this line: #22712 (comment)
(The reason this worked for S3 Bucket is because buckets are global resources, therefore, CDK assumes a bucket is in the same account as the principal and skip step 2.)
With the above thinking, I think we should update the code in class Grant
where it throws a warning if the code is trying to get the principals out of a not-real-principal (i.e. Managed Policy, Policy, Group), then skip adding resource based policy. The warning should notify customers that resource based policy will not be added (which is what customers would normally expect.)
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.
Crossposting analysis from the original issue. The problem is with how import resolves differently for Bucket vs TableV2. The new resources creation usecase passes correctly.
I think this will fix the other usecases where the imports are "inconsistent". Also was trying to avoid warnings.
Allowing grants on managed policy doesnt mean anything unless its attached to a principal or user or role.
TableName: this.resourceStack.table.tableName, | ||
}, | ||
})); | ||
} |
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.
The assertions seem to check CFN created the resources correctly. These should be CFN service assertions imo. In CDK integ tests, I think it is more important to check if CDK added the correct permissions that the integration need. In other words, we should invoke the infrastructure somehow to trigger runtime execution that exercises the permission added by CDK.
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.
Agreed. This particular check is to check for regressions in future where resource creation might be affected through a change. I can add a check with a managed policy attachment. Let me do that.
Closes #32795.
Reason for this change
Before this change, ManagedPolicy explicitly throw and error for statements like following for TableV2. This was supported in v2.131.
Description of changes
What code changes did you make?
policyFragment
now returns and empty PrincipalPolicyFragment. This enables the user to attach actions to policies. The generated CFN template will now contain the required actions, effects and resources blocks. It will still require the app to attach this policy to a Principal, user or role for it to work.
Have you made any important design decisions?
See above.
What AWS use cases does this change enable? To enable the use cases, which AWS service features are utilized?
Restores parity with a feature until 2.131 which allowed CDK apps to grant actions on resources to generate Policy Fragments for managed resources.
Describe any new or updated permissions being added
None
Description of how you validated changes
Yes to Both.
Integration test has been majorly refactored to do deployment checks as well in addition to the CFN checks.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license