-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
(aws-iam): Minimizing assume policies with conditions results in error #28713
(aws-iam): Minimizing assume policies with conditions results in error #28713
Comments
Hi I was not able to reproduce this error with the given snippet: const principal = new PrincipalWithConditions(
new ServicePrincipal("scheduler.amazonaws.com"),
{
StringEquals: {
"aws:SourceAccount": Stack.of(this).account,
},
}
);
const role = new Role(this, "Role", {
assumedBy: principal,
});
role.assumeRolePolicy?.addStatements(
new PolicyStatement({
effect: Effect.ALLOW,
principals: [principal],
actions: ["sts:AssumeRole"],
})
); I can synthesize with no error "Resources": {
"Role1ABCC5F0": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Statement": [
{
"Action": "sts:AssumeRole",
"Condition": {
"StringEquals": {
"aws:SourceAccount": <MY_AWS_ACCOUNT_ID>
}
},
"Effect": "Allow",
"Principal": {
"Service": "scheduler.amazonaws.com"
}
}
],
"Version": "2012-10-17"
}
},
"Metadata": {
"aws:cdk:path": "DummyStack/Role/Resource"
}
}, And I have the feature flag enabled in cdk.json "@aws-cdk/aws-iam:minimizePolicies": true, |
Thanks for taking a look. I have created a repository containing a dev container file here. Can you check if you can reproduce the error within the dev container? I have tried different CDK versions and 2.117.0 and older appear to work, while 2.118.0 and higher fail to synth. |
I'm getting the same error when upgrading from CDK 2.110.0 -> 2.126.0 (both CLI and aws-cdk-lib). The error message is identical to that in the OP, with the exception that mine quotes my AWS account as SourceAccount, rather than the Token directive. |
@Sordie thank you so much for providing the repo. I'm able to replicate the error. |
Having the same issue exactly when using |
@wtfzambo The work around I am using is to create my own role and pass it to all the scheduler targets. |
Policy minimizer seems to handle Works as expected: import * as cdk from 'aws-cdk-lib';
import * as iam from 'aws-cdk-lib/aws-iam';
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const principal = new iam.ServicePrincipal('dummy.amazonaws.com', {
conditions: { StringEquals: { 'aws:SourceAccount': stack.account } },
});
const role = new iam.Role(stack, 'Role', { assumedBy: principal });
role.assumeRolePolicy!.addStatements(new iam.PolicyStatement({
effect: iam.Effect.ALLOW,
principals: [principal],
actions: ['sts:AssumeRole'],
})); Throws Resolution Error: import * as cdk from 'aws-cdk-lib';
import * as iam from 'aws-cdk-lib/aws-iam';
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const principal = new iam.PrincipalWithCondition(new iam.ServicePrincipal('dummy.amazonaws.com'), {
StringEquals: { 'aws:SourceAccount': stack.account }
});
const role = new iam.Role(stack, 'Role', { assumedBy: principal });
role.assumeRolePolicy!.addStatements(new iam.PolicyStatement({
effect: iam.Effect.ALLOW,
principals: [principal],
actions: ['sts:AssumeRole'],
})); aws-cdk version is 2.146.0 |
I traced the code. The root cause is not in merge phase. Given const servicePrincipal = new iam.ServicePrincipal('dummy.amazonaws.com')
const principal = new iam.PrincipalWithConditions(servicePrincipal, { StringEquals: { 'aws:SourceAccount': stack.account } }); The constructor of const role = new iam.Role(stack, 'Role', { assumedBy: principal }); calles:
At the time, doc.statements[0]._principals is role.assumeRolePolicy.addStatements(new PolicyStatement({ principals: [principal], ... }) /* => st2 */) calls:
At this time, doc.statements[1]._principals is in merge phase:
|
@pahud Would you please focus this issue now? |
…ror during synth (#30634) ### Reason for this change Creating multiple `Schedule`s causes Resolution Error during synth. This PR does not fix the root cause (discussing at #28713), but apply a workaround to prevent the error. ### Description of changes Use `ServicePrincipal` with conditions directly, instead of `PrincipalWithConditions`. ### Description of how you validated changes Added a feature flag `{"@aws-cdk/aws-iam:minimizePolicies":true}` to unit tests. Resolution errors occur before fix. No errors occur after fix. ## Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Describe the bug
When using the AWS CDK with the
"@aws-cdk/aws-iam:minimizePolicies": true
setting, an error is encountered when creating aRole
with aPrincipalWithConditions
. The error suggests that the principals in aPolicyStatement
must have the same conditions, even though they do have the same conditions.Expected Behavior
The AWS CDK should successfully create a Role with a PrincipalWithConditions, even when the
"@aws-cdk/aws-iam:minimizePolicies"
setting is set to true. The conditions specified for the principals in thePolicyStatement
should be correctly merged without causing an error.Current Behavior
The following error is throw:
Reproduction Steps
Possible Solution
I think the issue might originate from here main/packages/aws-cdk-lib/aws-iam/lib/private/merge-statements.ts:54, but I can't pinpoint it exactly.
Additional Information/Context
It's worth noting that this issue has downstream effects, impacting the functionality of
aws-scheduler-targets
when using the same lambda with two schedulers.CDK CLI Version
2.121.1
Framework Version
No response
Node.js Version
18.19.0
OS
macOS 14.2.1
Language
TypeScript
Language Version
No response
Other information
No response
The text was updated successfully, but these errors were encountered: