Skip to content

Conversation

@Tietew
Copy link
Contributor

@Tietew Tietew commented Jan 17, 2025

Issue # (if applicable)

Closes #32980.

Reason for this change

lambda.Function.grantInvoke() throws an error when a ManagedPolicy or a Policy is passed.
It should add a policy statement to grant lambda:InvokeFunction on the policy document.

Description of changes

Core changes in (Managed)PolicyGrantPrincipal internal classes

Since grantInvoke() wants policyFragment.conditions, policyFragment now returns a PolicyFragment object with the principal ARN refers a token.
When the grantPrincipal is used as a resource policy, the token will cause a resolution error.

Additional changes

see also rix0rrr's comment

  • assumeRoleAction now throws an error instead of 'sts:AssumeRole'.
  • Updated the error message.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

  • Added/updated tests to verify grantInvoke() works.
  • Updated unit tests to verify app.synth() throws instead of Grant.addTo...().
  • Some class DummyResource implements IResourceWithPolicy are replaced by S3 bucket to ensure to create a resource policy (S3 bucket policy).

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team January 17, 2025 08:51
@github-actions github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. p2 labels Jan 17, 2025
@codecov
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.20%. Comparing base (5eeee75) to head (cee08ae).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #32984   +/-   ##
=======================================
  Coverage   82.20%   82.20%           
=======================================
  Files         119      119           
  Lines        6862     6862           
  Branches     1158     1158           
=======================================
  Hits         5641     5641           
  Misses       1118     1118           
  Partials      103      103           
Flag Coverage Δ
suite.unit 82.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk ∅ <ø> (∅)
packages/aws-cdk-lib/core 82.20% <ø> (ø)

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 17, 2025
@aws-cdk-automation
Copy link
Collaborator

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
@aws-cdk-automation
Copy link
Collaborator

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.

5d
5d previously requested changes May 15, 2025
* Adds an IAM statement to the policy document.
*/
public addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult {
this.addStatements(statement);
Copy link
Member

Choose a reason for hiding this comment

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

Best Practices for AWS IAM Managed Policies

From a best practices perspective, an AWS IAM ManagedPolicy functions as a shared state. Since it can be attached to multiple entities—users, roles, and groups—any change to the policy affects all consumers simultaneously.

The recommended approach is to define all required permissions at creation time and treat the policy as immutable throughout its lifecycle. This offers several key benefits:

  1. Predictability
    Changes to a shared policy propagate instantly, which can unintentionally alter access across your environment.

  2. Versioning Complexity
    Although AWS supports managed policy versioning, relying on it for rollback or audits introduces additional complexity.

  3. Least Privilege Enforcement
    Updating a shared policy risks granting broader permissions than intended.

  4. Governance and Control
    Because of their wide impact, shared policies should be subject to formal review and change control processes.

💡 Note: If a policy truly needs to change after creation, it may indicate the need for a dedicated IAM role or a purpose-built policy to better isolate and manage those permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@5d Thank you for reviewing.

This method is required by IPrincipal.
Constructing permissions step by step (calling addStatements multiple times) in CDK apps does not mean the managed policy is to be mutated after creation.

Are you saying that we should avoid to expose this method directly in ManagedPolicy, right?

Copy link
Member

Choose a reason for hiding this comment

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

The fundamental issue is that Policy and ManagedPolicy are not principals in the AWS IAM model. In AWS, policies are permission documents that are attached to principals (such as users, roles, or services), but they are not principals themselves.

By implementing the IPrincipal interface on Policy and ManagedPolicy, the CDK introduces a conceptual mismatch with the underlying AWS service model. While this implementation may add some convenience, such as allowing policies to be passed directly to grant methods, it creates an abstraction that does not align with how IAM actually works. This can lead to confusion for users who understand the underlying AWS IAM concepts.

The concern here is not primarily about method mutability, but about the architectural decision to make policy objects implement an interface meant for principals, when policies are fundamentally not principals in AWS IAM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation.
I'll withdraw implementing IPrincipal directly, and work on them for a few days.

@5d 5d self-assigned this May 15, 2025
@mergify mergify bot dismissed 5d’s stale review May 21, 2025 07:01

Pull request has been modified.

@Tietew Tietew force-pushed the iam-fix-policy-fragment branch from 1679955 to dc8b2dc Compare May 21, 2025 07:10
@github-actions github-actions bot added the effort/medium Medium work item – several days of effort label May 21, 2025
@Tietew
Copy link
Contributor Author

Tietew commented May 21, 2025

@5d
I've made following changes:

  • Revert implementing IPrincipal directly to (Managed)Policy constructs.
  • Move grantPrincipal implementations ((Managed)PolicyGrantPrincipal internal classes) to private subdir in order to be referred from policy-statement.ts

And also I've updated the PR description.

@5d
Copy link
Member

5d commented May 21, 2025

@5d I've made following changes:

  • Revert implementing IPrincipal directly to (Managed)Policy constructs.
  • Move grantPrincipal implementations ((Managed)PolicyGrantPrincipal internal classes) to private subdir in order to be referred from policy-statement.ts

And also I've updated the PR description.

Sorry @Tietew, I think there may be a misunderstanding. In your latest commit, it looks like you're treating Policy and ManagedPolicy as if they can behave like principals. However, this doesn't align with how AWS IAM works.

  • Policies are collections of permission statements that define what actions are allowed or denied. They don’t represent an identity and cannot be granted permissions themselves.
  • Principals (such as users, roles, or groups) are entities that receive permissions through policies.

In IAM terms: principals define who can act, while policies define what actions are allowed.

Could you clarify the reasoning behind granting permissions to a policy object? In what scenarios would this be necessary?

@5d 5d removed their assignment May 31, 2025
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@Abogical Abogical assigned rix0rrr and unassigned rix0rrr Oct 30, 2025
rix0rrr
rix0rrr previously approved these changes Oct 30, 2025
@mergify
Copy link
Contributor

mergify bot commented Oct 30, 2025

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

@Abogical Abogical assigned Abogical and rix0rrr and unassigned Abogical Oct 30, 2025
@mergify mergify bot dismissed rix0rrr’s stale review October 30, 2025 12:40

Pull request has been modified.

@Abogical
Copy link
Member

Re-adding approval of @rix0rrr after merge conflict fix.

@mergify
Copy link
Contributor

mergify bot commented Oct 30, 2025

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

@mergify mergify bot merged commit a07d75a into aws:main Oct 30, 2025
21 of 23 checks passed
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2025
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 30, 2025
@Tietew Tietew deleted the iam-fix-policy-fragment branch October 31, 2025 01:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

5 participants