Skip to content
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-cdk): Add session tags when CDK assumes an IAM Role. #26157

Open
2 tasks
j5nb4l opened this issue Jun 29, 2023 · 7 comments
Open
2 tasks

(aws-cdk): Add session tags when CDK assumes an IAM Role. #26157

j5nb4l opened this issue Jun 29, 2023 · 7 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@j5nb4l
Copy link

j5nb4l commented Jun 29, 2023

Describe the feature

Provide the ability to add session tags when assuming the CDK IAM Roles.

Use Case

Our team uses a central S3 bucket where all assets are uploaded for CloudFormation to use, as we have a lot of accounts and managing per-account/per-region buckets would be less than ideal. In order to prevent pipeline builds from accidentally overwriting assets uploaded by another, we use bucket policies to restrict the bucket prefix that any given session created by CI/CD server has access to. That way the CI/CD server must use the correct session tag to be granted access to prefix where the assets are uploaded. For example, here is what a simplified statement in the bucket policy looks like:

    {
      "Sid": "AllowReadWriteCrossAccountViaPrincipalTag",
      "Effect": "Allow",  
      "Principal": "*",
      "Action": [
        "s3:GetObject",
        "s3:PutObject"
      ],  
      "Resource": "arn:aws:s3:::bucketName/us-east-1/${aws:PrincipalTag/serverId:uniqueId}/*",
      "Condition": {
        "StringLike": {      
          "aws:arn": [
            "arn:aws:sts::<account-id>:assumed-role/cicd-role/*",
           ]
        }
      }
    }

We would like to be able to follow this same access pattern when CDK uploads assets to the S3 bucket from our pipeline; however, there does not seem to be any way to configure session tags when using the DefaultStackSynthesizer.

Proposed Solution

No response

Other Information

No response

Acknowledgements

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

CDK version used

2.78.0 (build 8e95c37)

Environment details (OS name and version, etc.)

MacOS Ventura 13.4.1 (22F82)

@j5nb4l j5nb4l added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jun 29, 2023
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Jun 29, 2023
@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jun 29, 2023
@peterwoodworth
Copy link
Contributor

We don't appear to support this, but this would be nice to have. Thanks for the request!

@ishug86
Copy link

ishug86 commented Aug 14, 2023

This is a very important feature which is missing. Would love to have it available soon.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 29, 2023

Hi @j5nb4l and @ishug86 Thanks for the feature request!

Could you both elaborate a bit further on your use case? We would like to understand the problem you are solving (trying to solve), how this feature would help with it, and how exactly you would be using the new feature.

Specifically, it sounds to me like you want to be able to instruct the aws-cdk CLI to set session tags when it assumes roles.
Is that correct? Are there any other assume role calls that would need the set session tags? How would you like to define the session tags, i.e. as a CLI command, in code, in a config file, etc.

Our team uses a central S3 bucket where all assets are uploaded for CloudFormation to use, as we have a lot of accounts and managing per-account/per-region buckets would be less than ideal

@j5nb4l This sounds like a non-default CDK bootstrap setup. Can you tell us more about how you currently achieve this? What are makes managing per-account/per-region buckets less than ideal for you?

@j5nb4l
Copy link
Author

j5nb4l commented Aug 30, 2023

Hi @mrgrain,

Could you both elaborate a bit further on your use case? We would like to understand the problem you are solving (trying to solve), how this feature would help with it, and how exactly you would be using the new feature.

The S3 buckets used to host CloudFormation assets uses Attribute-based Access Controls to restrict the access granted to STS session based on session tags.

Specifically, it sounds to me like you want to be able to instruct the aws-cdk CLI to set session tags when it assumes roles.
Is that correct?

Yes, that's correct.

Are there any other assume role calls that would need the set session tags? How would you like to define the session tags, i.e. as a CLI command, in code, in a config file, etc.

This is a great question, and the reason why I did not attempt to implement it. From my perspective, a CLI argument, like --session-tag tag:value, would probably suffice; however, I think the ideal implementation should support all of the above. In other words, a CLI argument to add session tags to every AssumeRole session created during the operation, and the ability to add session tags through options provided to the stack synthesizer.

Our team uses a central S3 bucket where all assets are uploaded for CloudFormation to use, as we have a lot of accounts and managing per-account/per-region buckets would be less than ideal

This sounds like a non-default CDK bootstrap setup. Can you tell us more about how you currently achieve this? What are makes managing per-account/per-region buckets less than ideal for you?

That was in reference to how it is done for native CloudFormation (i.e., without CDK) where the AssumeRole sessions are created using the aws-cli. At the moment, we are using the CliCredentialsStackSynthesizer to replicate that behavior as an interim solution; however, it is not an ideal long-term solution.

I hope I adequately answered your questions. Please let me know if you need any clarifications or further information.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 30, 2023

Thanks for the details. That all makes sense and will help us to prioritize the feature.

It does sounds like you don't have this implemented yet with CDK, presumingly because you are blocked by the lack of session tags? I'm a bit worried that you are going to run into more issues with this approach and while session tags might be a useful thing on it's own, it would potentially not be very useful without further changes to Synthesizer, Bootstrapping or both.


I expect the implementation to follow closely what we already do for externalId:

readonly fileAssetPublishingExternalId?: string;

readonly imageAssetPublishingExternalId?: string;

It probably makes the most/only sense to have this implemented on the Synthesizer level, since the CLI can be used with other Synthesizers as well that don't assume any roles.

@j5nb4l
Copy link
Author

j5nb4l commented Aug 30, 2023

Thanks for the details. That all makes sense and will help us to prioritize the feature.

It does sounds like you don't have this implemented yet with CDK, presumingly because you are blocked by the lack of session tags? I'm a bit worried that you are going to run into more issues with this approach and while session tags might be a useful thing on it's own, it would potentially not be very useful without further changes to Synthesizer, Bootstrapping or both.

I can certainly understand what you mean; however, I think it would be very useful to have the ability to add session tags even if they were not defined in the synthesizer or when trying to deploy an app that was already synthesized (e.g., cdk deploy --app=existing.cdk.out.dir --session-tag key:value). In those situations, the argument could be ignored if no roles are being assumed.

I expect the implementation to follow closely what we already do for externalId:

readonly fileAssetPublishingExternalId?: string;

readonly imageAssetPublishingExternalId?: string;

It probably makes the most/only sense to have this implemented on the Synthesizer level, since the CLI can be used with other Synthesizers as well that don't assume any roles.

I think the way that externalId was implemented would work fine. The new property would just have to take an object with key value pairs for each session tag instead of a string, right?

@mrgrain
Copy link
Contributor

mrgrain commented Aug 30, 2023

I think the way that externalId was implemented would work fine. The new property would just have to take an object with key value pairs for each session tag instead of a string, right?

Yes, pretty much that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

No branches or pull requests

4 participants