(dynamodb): grant calls no longer working when passed a ManagedPolicy

[wilhen01]

Describe the bug

Previously, it's been possible to use .grant functions on dynamoDB tables and pass them a ManagedPolicy (which is allowed by the type system since ManagedPolicy implements IGrantable).

Under 2.174.0, that results in an error:

Cannot use a ManagedPolicy as the 'Principal' or 'NotPrincipal' in an IAM Policy.

This is a change in behaviour, and it's also inconsistent with e.g. S3 where it's still possible to call bucket.grantReadWrite(managedPolicy);

Regression Issue

• Select this option if this issue appears to be a regression.

Last Known Working CDK Version

2.131.0

Expected Behavior

.grant functions for dynamo tables work when passed a ManagedPolicy and add the appropriate policy statements.

Current Behavior

Error thrown: Cannot use a ManagedPolicy as the 'Principal' or 'NotPrincipal' in an IAM Policy

Reproduction Steps

const table = TableV2.fromTableArn(
  this,
  'Table',
  arnForTable,
);
table.grantReadWriteData(managedPolicy);

Error is visible in CDK unit tests which verify the managed policy composition.

Possible Solution

No response

Additional Information/Context

I'm not sure if the new behaviour is correct in that a ManagedPolicy is not in fact a Principal, and hence the previous behaviour shouldn't have been allowed, but it should be consistent and obvious from documentation where you can and can't use the convenience grant functions. At present they work in this scenario for other library areas e.g. S3.

CDK CLI Version

2.174.0

Framework Version

No response

Node.js Version

18

OS

Mac OS

Language

TypeScript

Language Version

5.x

Other information

No response

[pahud]

Can you show me how do you define managedPolicy?

if you use fromXxx() method, you get IManagedPolicy rather than ManagedPolicy and IManagedPolicy does not implement IGrantable.

[wilhen01]

@pahud it's defined within the same CDK stack I'm using the .grant calls in via new ManagedPolicy. I'm not doing any casts on it in order to pass it into the grant calls either, so it clearly implements IGrantable. The code looks roughly like:

const policy = new ManagedPolicy(this, 'StandardPolicy', {
      managedPolicyName: 'StandardPolicy',
      description:
        'Grants team members permissions to perform BAU tasks such as lambda invocation and reading secrets.',
});

...

const table = TableV2.fromTableArn(
  this,
  'Table',
  arnForTable,
);
table.grantReadWriteData(policy);

There are a number of other calls in the stack to add other statements to the policy (many of which are constructed manually via new PolicyStatement rather than using .grant convenience functions) but you get the picture.

[pahud]

Yes I am seeing

Error: Cannot use a ManagedPolicy 'DummyStack2/StandardPolicy' as the 'Principal' or 'NotPrincipal' in an IAM Policy
at ManagedPolicyGrantPrincipal.get policyFragment [as policyFragment] (/***/node_modules/aws-cdk-lib/aws-iam/lib/managed-policy.js:1:5464)

The error is thrown from

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

Line 339 in bbdd42c

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

Reason:

// 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

which was fixed in this PR.

so it should be a bug fix not a breaking change.

[wilhen01]

I don't think that PR is the root cause of this bug? If I'm reading it correctly that PR adds IGrantable to ManagedPolicy, but my code was working as of 2.131.0 and ManagedPolicy implements IGrantable in that version of the library so I think something else has changed between 2.131.0 and the current version to cause this issue...

[pahud]

If we read the git blame, that PR introduced ManagedPolicyGrantPrincipal that implements IPrincipal.

According to your error message:

Cannot use a ManagedPolicy as the 'Principal' or 'NotPrincipal' in an IAM Policy

I am pretty sure it was thrown from here:

https://github.com/aws/aws-cdk/blame/bbdd42c8f45916d5c6945f3429916f6199d2ec66/packages/aws-cdk-lib/aws-iam/lib/managed-policy.ts#L335C14-L340

I will bring this up to the team for further inputs.

[wilhen01]

Cool, thanks @pahud!

[QuantumNeuralCoder]

Hey
This seems to be the correct behavior as per the comment on the implementation ie
"managed policy cannot be used as a principal of a policy document".

Here are some references

AWS IAM Documentation: "Specifying a Principal"
This page explicitly lists the entities that can be specified as a principal, such as AWS accounts, IAM users, IAM roles, federated users, and AWS services. Managed Policies are not listed here, so they cannot be used as principals.

AWS IAM Documentation: "Managed Policies and Inline Policies"
Managed Policies are reusable policy documents that can be attached to multiple identities (like IAM users, groups, or roles). They are not identities themselves, which is why they cannot be used as principals in an IAM policy.

Please let us know if there still a concern.

[wilhen01]

@QuantumNeuralCoder as noted in the original bug description under Additional Information/Context:

I'm not sure if the new behaviour is correct in that a ManagedPolicy is not in fact a Principal, and hence the previous behaviour shouldn't have been allowed, but it should be consistent and obvious from documentation where you can and can't use the convenience grant functions. At present they work in this scenario for other library areas e.g. S3.

If this is the correct behaviour then it should be implemented consistently across the various parts of CDK, which isn't the case at present.

[pahud]

If this is the correct behaviour then it should be implemented consistently across the various parts of CDK, which isn't the case at present.

If you find any existing code of the latest release not following this behavior, that should be a bug we need to get it immediately fixed. Please let us know. Thank you.

[wilhen01]

If this is the correct behaviour then it should be implemented consistently across the various parts of CDK, which isn't the case at present.

If you find any existing code of the latest release not following this behavior, that should be a bug we need to get it immediately fixed. Please let us know. Thank you.

@pahud again, back to the original description:

This is a change in behaviour, and it's also inconsistent with e.g. S3 where it's still possible to call bucket.grantReadWrite(managedPolicy);

[QuantumNeuralCoder]

@wilhen01
Your point seems to be valid. Let me discuss internally and circle back. But blocking granting of permissions to managed policies which are essentially container of permissions is the right behavior.

[QuantumNeuralCoder]

We discussed this internally. This is the original source of this feature. Here the grant statements effectively mutate the PolicyDocument itself. Its effective only after a principal is specified or until you attach the policy to an identity.
We will bring TableV2 to parity with this.

[Tietew]

Hi, folks.

It worked well when I made the PR...
Should ManagedPolicy and Policy be handled as same as Group?

Group.policyFragment returns a dummy policy fragment:

aws-cdk/packages/aws-cdk-lib/aws-iam/lib/group.ts

Lines 82 to 84 in 2b2443d

	public get policyFragment(): PrincipalPolicyFragment {
	return new ArnPrincipal(this.groupArn).policyFragment;
	}

Adding Group to resource policy is blocked in PolicyStatement:

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

Lines 240 to 244 in 2b2443d

	private validatePolicyPrincipal(principal: IPrincipal) {
	if (principal instanceof Group) {
	throw new Error('Cannot use an IAM Group as the \'Principal\' or \'NotPrincipal\' in an IAM Policy');
	}
	}

[Tietew]

@wilhen01
I can reproduce this issue when the table is cross-account from the managed policy.

const managedPolicy = new iam.ManagedPolicy(this, 'ManagedPolicy');
const table1 = dynamodb.TableV2.fromTableName(this, 'Table1', 'same-account-table');
table1.grantReadWrite(managedPolicy); // OK

const table2 = dynamodb.TableV2.fromTableArn(this, 'Table2', 'arn:aws:dynamodb:region:other-account:table/cross-account-table');
table2.grantReadWrite(managedPolicy); // Fail

This behavior is intended because you cannot specify a ManagedPolicy as a principal of a resource policy.

Note: when the stack's account is unspecified (rendered as { "Ref": "AWS::Account" }) and the account part of the table arn is concrete value, CDK will assume cross-account.

You can specify account of Stack to avoid above senario:

const stack = new cdk.Stack(app, 'Stack', { env: { account: '12345678' } });
const table = dynamodb.TableV2.fromTableArn(stack, 'Table', 'arn:aws:dynamodb:region:12345678:table/same-account-table');
table.grantReadWrite(managedPolicy); // OK

[wilhen01]

@Tietew I'm specifying an account number in the stack, I'm referencing the table ARNs via imports, and none of it is cross-account anyway. It fails with the error given in the original bug report at the top of this issue. There's definitely some form of regression here.

[QuantumNeuralCoder]

A new table creation works as expected. This is specific to usecase where we import the table using fromTableArn. Because we resolve the account the addToPrincipalOrResource proceeds beyond the check (grant.js:141) into creating new PolicyStatement and fails as we explicitly throw the error for ManagedPolicy.
So for a Managed Policy,
Below works...

const itable = new TableV2(this, 'MyTable', {
  tableName: 'my-table-name', // Optional: If not specified, CDK will generate a name
  partitionKey: {
    name: 'id',
    type: AttributeType.STRING
  },
  // Optional configurations
  deletionProtection: true,
  pointInTimeRecovery: true,
});

  itable.grantReadWriteData(policy);
  }

Below does not

    const table = TableV2.fromTableArn(
      this,
      'Table',
      arnForTable,
    );
    table.grantReadWriteData(policy);

We are looking into revising this behavior.