(cli): Review permissions structure for new changeset-based diffs

[blimmer]

Describe the feature

I love the goal of the new cdk diff behavior that creates a CloudFormation changeset to more accurately show information about what will happen if you cdk deploy.

However, I've encountered many problems with existing IAM Roles and Permission Sets I've created for working with CDK.

In the past, I've always been able to create two roles: one for diff-ing and one for deploy-ing. These roles only needed to provide iam:AssumeRole for the following roles created by the CDK bootstrap process:

Role	CDK Bootstrap Roles to Assume
Diff	lookup-role
Deploy	cfn-exec-role lookup-role deploy-role file-publishing-role image-publishing-role

Since CDK has started generating CloudFormation diffs, I've been receiving and error that, during cdk diff, the role needs to assume the CDK deploy-role:

Before

> yarn cdk diff --no-change-set
yarn run v1.22.21
warning package.json: No license field
$ cdk diff --no-change-set
Stack TestCdkStack
There were no differences

✨  Number of stacks with differences: 0

✨  Done in 3.32s.

After

> cdiff
yarn run v1.22.21
warning package.json: No license field
$ cdk diff
Stack TestCdkStack
current credentials could not be used to assume 'arn:aws:iam::12345678910:role/cdk-hnb659fds-deploy-role-12345678910-us-west-2', but are for the right account. Proceeding anyway.
Could not create a change set, will base the diff on template differences (run again with -v to see the reason)
There were no differences

✨  Number of stacks with differences: 0

✨  Done in 3.76s.

So, I'm opening this issue suggesting that the CDK Bootstrap roles be revisited with the new CFN Changeset behaviors.

Use Case

I'm using GitHub Actions OpenID Connect roles to run cdk diff on PRs and cdk deploy on merge to my main branch. Then, I use Trust Relationships to only allow assuming the deploy role from Action Workflows run from the main branch:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "Federated": "arn:aws:iam::12345678910:oidc-provider/token.actions.githubusercontent.com"
            },
            "Action": "sts:AssumeRoleWithWebIdentity",
            "Condition": {
                "StringEquals": {
                    "token.actions.githubusercontent.com:aud": "sts.amazonaws.com"
                },
                "StringLike": {
                    "token.actions.githubusercontent.com:sub": "repo:blimmer/my-repo:ref:refs/heads/main"
                }
            }
        }
    ]
}

The diff role can be assumed by any branch, since it's assumed to be read-only. However, with the recent CloudFormation changeset diffs, these roles no longer work as expected. To generate the changelogs, they appear to need elevated permissions, including access to the deploy-role, which has the ability to cloudformation:DeleteStack and other destructive behaviors.

These OpenID Connect roles are just one example. You could also imagine Permission Sets for Developers that allow them to run cdk diff from their local computers, but they should not be allowed to run cdk deploy.

Proposed Solution

No matter the solution, I think the goal should be that CDK users can define IAM roles/policies that slot users into two buckets: people who can do non-destructive things (like cdk diff) and those who can do everything (e.g., cdk deploy).

Could we update the lookup-role to allow the cloudformation actions needed to create the changeset for diffing purposes? I think that would just include adding cloudformation:CreateChangeSet and cloudformation:DeleteChangeSet? Then, the cdk diff should not try to assume the deploy-role anymore.

Other Information

I originally thought this issue was only with the App Staging synthesizer (see #28816), but it feels like there's some overlap here. We might want to close #28816 in favor of this issue?

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

CDK version used

2.136.0 (build 94fd33b)

Environment details (OS name and version, etc.)

MacOS Sonoma

[scanlonp]

Hey @blimmer, thanks for opening this issue with us. A fair number of users have use cases mirroring yours in which they scope certain environments to make diff calls with lookup permissions, and others to make deploy calls with the deploy permissions. The new diff behavior does not fit nicely in that setup.

We have three ways to solve this:

1. Broaden the permissions of the lookup role to include CreateChangeSet. This is not viable, because the lookup role should, on principle, not create anything.
2. Create a new role that has the permissions of the lookup role + CreateChangeSet + delete the changeset.
3. Just use the deploy role, which already has this permission.

We didn't do (2) initially because we needed to make DeleteStack calls (since we were creating changesets for undeployed stacks), and at that point the deploy role was close enough.

However, we are no longer creating changesets for new stacks, so we do not make DeleteStack calls, only DeleteChangeSet. This makes (2) a viable option going forward.

To use your table, your setup could look something like this:

Role	CDK Bootstrap Roles to Assume
Lookup	lookup-role
Diff	lookup-role cfn-changeset-review-role
Deploy	cfn-exec-role lookup-role deploy-role file-publishing-role image-publishing-role

Where the cfn-changeset-review-role would have CreateChangeSet and DeleteChangeSet permissions, but not ExecuteChangeSet.

[blimmer]

I like Option 2, as well. Allowing diff by granting access to lookup-role and cfn-changeset-review-role is straightforward, easy to understand, and adds greater security than allowing access to the deploy-role for diffing. +1

[scanlonp]

Got it, will keep you updated where we go with this.

[mrtimp]

We have run into this with the same usecase where we have a PR role used by GitHub Actions running https://github.com/karlderkaefer/cdk-notifier

[sakurai-ryo]

WIP

[sakurai-ryo]

Hi @blimmer @scanlonp
The cfn template must be uploaded to S3 when creating the changeset.
(strictly only if the size of the template is larger than 51,200 bytes)
https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_CreateChangeSet.html#:~:text=Required%3A%20No-,TemplateBody,-A%20structure%20that

I think we will need to assume the file-publishing-role to upload the template to S3. Is this acceptable?

Role	CDK Bootstrap Roles to Assume
Lookup	- lookup-role
Diff	- lookup-role - cfn-changeset-review-role - file-publishing-role
Deploy	- cfn-exec-role - lookup-role - deploy-role - file-publishing-role - image-publishing-role

[blimmer]

@sakurai-ryo - could we have the new cfn-changeset-review-role have the proper s3 permission to upload the changesets? I slightly prefer that to giving access to file-publishing-role. If that's not feasible, it's OK with me to need file-publshing-role.

[dloez]

Is not the bucket used to upload the cfn templates the same used to upload different CDK assets, such zip file for bundled lambdas (NodejsFunction)? Just wondering what issues could arise, maybe just a warning in the docs is more than enough.

[sakurai-ryo]

Thanks @blimmer
Yes, we can configure the new cfn-changeset-review-role to have the necessary S3 permissions to upload the changesets.

I forgot that the file-publishing-role has various permissions such as deleting s3 objects.
It is definitely more secure to attach only necessary permissions such as upload to the cfn-changeset-review-role.

[sakurai-ryo]

Hi @scanlonp
Really sorry for mentioning you.
Although the build was successful and I have submitted the Exemption Request in my PR, the pr-linter/exemption-requested label has not been attached, so the pr/needs-maintainer-review label is not being attached. Could you please apply the label directly?

[kylebjordahl]

We're also very interested in this; looks like the PR from @sakurai-ryo is getting closed for staleness though.

@scanlonp is there something missing that blocks getting the work merged? Glad to contribute if there is something more to be done.

[sakurai-ryo]

I missed the message from the aws-cdk-automation bot.
I will re-create the PR again later.

[scanlonp]

@sakurai-ryo, I'll re-open the PR and make sure it stays open. Thanks for opening this PR! I did see it, though I have not had a chance to give a thorough review.

@kylebjordahl, not anything missing or blocking it. This is a larger change, so we will want to take some time for the team to review and discuss it, but it is certainly on our radar!