feat(iam): implement IGrantable to Policy and ManagedPolicy #22712

Tietew · 2022-10-31T12:15:51Z

Fixes #10308

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-10-31T12:16:08Z

packages/@aws-cdk/aws-iam/lib/managed-policy.ts

packages/@aws-cdk/aws-iam/lib/lazy-role.ts

Pull request has been modified.

Tietew · 2022-11-11T05:58:56Z

I've marked as draft due to incomplete; i.e. s3.Bucket.grantReadWrite calls addToPrincipalOrResource.
Working.

aws-cdk-automation · 2022-11-12T00:10:18Z

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 · 2022-11-13T00:10:47Z

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 · 2022-11-14T00:10:29Z

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.

…urce`

aws-cdk-automation · 2022-11-25T00:10:21Z

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 · 2022-11-26T00:08: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.

aws-cdk-automation · 2022-11-27T00:10:27Z

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 · 2022-11-28T00: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.

dan-lind · 2022-11-28T11:45:24Z

@Tietew Is this still a draft? Was wondering just I just ran into the issue of ManagedPolicy not implementing IGrantable

Tietew · 2022-12-01T08:00:47Z

I researched usage of grant* methods.

IAM group and IPrincipal

Group implements IPrincipal but an IAM group itself is not an actual principal.
See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#Principal_specifying
When trying to pass Group as a principal of a resource-based policy, PolicyStatement.validatePolicyPrincipal() throws an error.
(checked by instanceof Group )

Policy and ManagedPolicy is also not a principal.

const stack1234 = new Stack(app, 'stack1234', { env: { account: '1234' } });
const group = new iam.Group(stack1234, 'group', { ... });
const mp = new iam.ManagedPolicy(stack1234, 'policy', { ... });

const stack5678 = new Stack(app, 'stack5678', { env: { account: '5678' } });
const s3 = new s3.Bucket(stack5678, 'bucket', { ... });
s3.grantRead(group); // fails: Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy
s3.grantRead(mp); // should fail too!

IPrincipal.policyFragment property will be referred when the grantee to a resource policy.
I think that it's the simple way to throw an error in the getter of the property to cause above failure.

list of grant* methods usage

simple

Only Grant.addToPrincipalPolicy() is called. (ignores policyFragment)

aws-apigateway
aws-apigatewayv2
aws-appmesh
aws-appsync
aws-backup
aws-cloudwatch
aws-codedeploy
aws-codeguruprofiler
aws-codepipeline-actions
aws-cognito
aws-efs
aws-elasticsearch
aws-events
aws-gamelift
aws-iotevents
aws-kinesisfirehose
aws-location
aws-neptune
aws-opensearchservice
aws-sagemaker
aws-stepfunctions

calls `addToPrincipalOrResource`

Following methods call addToPrincipalOrResource(). grantee will be added to resource policy when cross-account.

aws-ecr: IRepository.grant*
aws-lambda: IFunction.grantInvoke IFunction.grantInvokeUrl IFunctionUrl.grantInvokeUrl
aws-logs: ILogGroup.grant*
aws-s3: IBucket.grant*
aws-secretsmanager: ISecret.grant*
aws-sns: ITopic.grantPublish
aws-sqs: IQueue.grant*

calls `addToPrincipalAndResource`

Only KMS calls addToPrincipalAndResource().
When the key has a default policy (trustAccountIdentities is true) and is not cross-region nor cross-account, addToPrincipalOrResource() is called. Otherwise addToPrincipalAndResource() is called.

aws-kms: IKey.grant*

delegate to S3 / Secrets Manager

Same restrictions as addToPrincipalOrResource()

aws-codebuild: IReportGroup.grantWrite
aws-glue: ITable.grantRead ITable.grantWrite ITable.grantReadWrite
aws-rds: IServerlessCluster.grantDataApiAccess

delegate to KMS

Same restrictions as KMS if encryption is enabled.

aws-ec2: IVolume.grantAttachVolume IVolume.grantAttachVolumeByResourceTag
aws-kinesis: IStream.grantRead IStream.grantWrite IStream.grantReadWrite
aws-secretsmanager: ISecret.grant*
aws-sqs: IQueue.grantConsumeMessages IQueue.grantSendMessage
aws-ssm: IParameter.grant*

special case

aws-iam: IRole.grant* (requires IPrincipal, but calls addToPrincipalPolicy() only)
aws-redshift: ITable.grant (requires redshift IUser)

Tietew · 2022-12-01T08:18:19Z

@dan-lind Thank you for catching! It's ready now.

aws-cdk-automation · 2022-12-10T00:08:39Z

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 · 2022-12-11T00:09:45Z

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.

toxygene · 2022-12-27T22:29:48Z

Is there anything else that needs to be done here? I'm eagerly awaiting this functionality and I'd hate to see the PR languish.

toxygene · 2023-01-30T18:52:53Z

Is there anything the community can do to get this PR approved? I'm very excited to see the functionality added to CDK and I'm afraid that progress on it has stalled.

comcalvi

This looks really good, and hopefully we can merge it soon! I have a conceptual and cosmetic comments

packages/@aws-cdk/aws-iam/lib/managed-policy.ts

comcalvi · 2023-02-02T22:43:57Z

packages/@aws-cdk/aws-iam/lib/managed-policy.ts

+  }
+
+  public get policyFragment(): PrincipalPolicyFragment {
+    throw new Error(`Cannot use a ManagedPolicy ${this._managedPolicy.node.path} as the 'Principal' or 'NotPrincipal' in an IAM Policy`);


Suggested change

throw new Error(`Cannot use a ManagedPolicy ${this._managedPolicy.node.path} as the 'Principal' or 'NotPrincipal' in an IAM Policy`);

throw new Error(`Cannot use a ManagedPolicy '${this._managedPolicy.node.path}' as the 'Principal' or 'NotPrincipal' in an IAM Policy`);

Can you add a comment above this error explaining why we need to throw it? It's not immediately clear

I believe policyFragment is currently the best location to raise an error when we try to add Policy/ManagedPolicy to a resource-based policy. Suggestions are welcome.

@Tietew I know this is an old thread but I have a question wanting to understanding better.

You mentioned that PolicyStatement.validatePolicyPrincipal() throws error for Group. So why not update PolicyStatement.validatePolicyPrincipal() to throw for Policy or Managed Policy as well?

packages/@aws-cdk/aws-iam/test/policy.test.ts

packages/@aws-cdk/aws-iam/test/managed-policy.test.ts

packages/@aws-cdk/aws-iam/lib/policy.ts

Pull request has been modified.

Tietew · 2023-02-03T10:14:12Z

@comcalvi Thank you for your review. updated.

comcalvi

Nice work!

mergify · 2023-02-08T00:40:02Z

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-02-08T01:20:19Z

AWS CodeBuild CI Report

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

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

mergify · 2023-02-08T01:20:31Z

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).

Tietew · 2023-02-08T02:42:10Z

#7448

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Oct 31, 2022

aws-cdk-automation requested a review from a team October 31, 2022 12:16

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Oct 31, 2022

TheRealAmazonKendra previously requested changes Oct 31, 2022

View reviewed changes

packages/@aws-cdk/aws-iam/lib/managed-policy.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-iam/lib/managed-policy.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-iam/lib/lazy-role.ts Outdated Show resolved Hide resolved

Tietew force-pushed the iam-policy-grantable branch 2 times, most recently from 6861ad6 to 653b0a2 Compare November 2, 2022 07:46

Tietew added 6 commits November 2, 2022 17:01

implement IGrantable on ManagedPolicy

4f9c339

implement IGrantable on Policy

35127ec

README

3debf52

add failures test cases

7071b04

revert argument type of IRole.grant*

1114859

private class instead of inline

6b2b29d

Tietew force-pushed the iam-policy-grantable branch from 653b0a2 to 6b2b29d Compare November 2, 2022 08:21

Tietew marked this pull request as draft November 11, 2022 05:55

Tietew added 3 commits November 14, 2022 11:31

assign principalAccount to be passed to `Grant.addToPrincipalOrReso…

feb676a

…urce`

Merge branch 'main' into iam-policy-grantable

94a2150

retrieve principalAccount from env

9ba22bd

Merge remote-tracking branch 'origin/main' into iam-policy-grantable

df13fad

dan-lind mentioned this pull request Nov 29, 2022

CDK Grants into AWS Managed Policy #7448

Closed

2 tasks

Tietew marked this pull request as ready for review December 1, 2022 08:01

Tietew requested a review from TheRealAmazonKendra December 1, 2022 08:01

Merge branch 'main' into iam-policy-grantable

baea213

Merge branch 'main' into iam-policy-grantable

6fdbace

comcalvi previously requested changes Feb 2, 2023

View reviewed changes

Merge branch 'aws:main' into iam-policy-grantable

ddb7cc3

Tietew added 2 commits February 3, 2023 18:40

add comments

27496b4

update tests

340e2bd

comcalvi approved these changes Feb 8, 2023

View reviewed changes

Merge branch 'main' into iam-policy-grantable

8647801

mergify bot merged commit d3df40f into aws:main Feb 8, 2023

Tietew deleted the iam-policy-grantable branch February 8, 2023 02:33

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Tietew mentioned this pull request Jan 17, 2025

iam: cannot pass iam.ManagedPolicy or iam.Policy to lambda.Function.grantInvoke #32980

Open

1 task

samson-keung mentioned this pull request Jan 27, 2025

fix(iam): managed-policy can again be granted actions on resources #33115

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iam): implement IGrantable to Policy and ManagedPolicy #22712

feat(iam): implement IGrantable to Policy and ManagedPolicy #22712

Tietew commented Oct 31, 2022 •

edited

Loading

gitpod-io bot commented Oct 31, 2022

Tietew commented Nov 11, 2022

aws-cdk-automation commented Nov 12, 2022

aws-cdk-automation commented Nov 13, 2022

aws-cdk-automation commented Nov 14, 2022

aws-cdk-automation commented Nov 25, 2022

aws-cdk-automation commented Nov 26, 2022

aws-cdk-automation commented Nov 27, 2022

aws-cdk-automation commented Nov 28, 2022

dan-lind commented Nov 28, 2022

Tietew commented Dec 1, 2022

Tietew commented Dec 1, 2022

aws-cdk-automation commented Dec 10, 2022

aws-cdk-automation commented Dec 11, 2022

toxygene commented Dec 27, 2022

toxygene commented Jan 30, 2023

comcalvi left a comment

comcalvi Feb 2, 2023

Tietew Feb 6, 2023

samson-keung Jan 27, 2025

Tietew commented Feb 3, 2023

comcalvi left a comment

mergify bot commented Feb 8, 2023

aws-cdk-automation commented Feb 8, 2023

mergify bot commented Feb 8, 2023

Tietew commented Feb 8, 2023

	throw new Error(`Cannot use a ManagedPolicy ${this._managedPolicy.node.path} as the 'Principal' or 'NotPrincipal' in an IAM Policy`);
	throw new Error(`Cannot use a ManagedPolicy '${this._managedPolicy.node.path}' as the 'Principal' or 'NotPrincipal' in an IAM Policy`);

feat(iam): implement IGrantable to Policy and ManagedPolicy #22712

feat(iam): implement IGrantable to Policy and ManagedPolicy #22712

Conversation

Tietew commented Oct 31, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Oct 31, 2022

Tietew commented Nov 11, 2022

aws-cdk-automation commented Nov 12, 2022

aws-cdk-automation commented Nov 13, 2022

aws-cdk-automation commented Nov 14, 2022

aws-cdk-automation commented Nov 25, 2022

aws-cdk-automation commented Nov 26, 2022

aws-cdk-automation commented Nov 27, 2022

aws-cdk-automation commented Nov 28, 2022

dan-lind commented Nov 28, 2022

Tietew commented Dec 1, 2022

IAM group and IPrincipal

list of grant* methods usage

simple

calls addToPrincipalOrResource

calls addToPrincipalAndResource

delegate to S3 / Secrets Manager

delegate to KMS

special case

Tietew commented Dec 1, 2022

aws-cdk-automation commented Dec 10, 2022

aws-cdk-automation commented Dec 11, 2022

toxygene commented Dec 27, 2022

toxygene commented Jan 30, 2023

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Feb 2, 2023

Choose a reason for hiding this comment

Tietew Feb 6, 2023

Choose a reason for hiding this comment

samson-keung Jan 27, 2025

Choose a reason for hiding this comment

Tietew commented Feb 3, 2023

comcalvi left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 8, 2023

aws-cdk-automation commented Feb 8, 2023

AWS CodeBuild CI Report

mergify bot commented Feb 8, 2023

Tietew commented Feb 8, 2023

Tietew commented Oct 31, 2022 •

edited

Loading

calls `addToPrincipalOrResource`

calls `addToPrincipalAndResource`