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

fix(cli): codepipeline cross account action fails writing output artifacts #6594

Merged
merged 2 commits into from
Mar 9, 2020

Conversation

NetaNir
Copy link
Contributor

@NetaNir NetaNir commented Mar 5, 2020

To allow cross account actions in codepipeline we create a role in the other account which is assumable by codepipeline. Since the artifacts are stored in an encrypted S3 bucket in the code pipeline account, the role in the other account must have permission to access the bucket and KMS key in the codepipeline account. To give the role permissions to access the bucket and key two type of policies are required:

  1. The policy defined on the bucket and key must give the other account permissions to perform all required actions.
  2. The role policy must allow all required actions

Policy 1 is defined by the codepipeline construct.
The role in the other account is created via the (new) bootstrap command and is defined as DeploymentActionRole in the bootstrap-template.json file. To satisfy 2, the policy attached to the role must allow the required S3 and KMS actions. The policy attached to the role was missing the required KMS actions to allow writing the output artifacts to the S3 bucket. This commits adds kms:Encrypt, kms:ReEncrypt , kms:GenerateDataKey to the DeploymentActionRole attached policy


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

@NetaNir NetaNir requested review from skinny85 and rix0rrr March 5, 2020 07:11
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 5, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 5761dfc
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@NetaNir NetaNir added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels Mar 5, 2020
@@ -337,7 +337,8 @@
"s3:GetObject*", "s3:GetBucket*",
"s3:List*", "s3:Abort*",
"s3:DeleteObject*", "s3:PutObject*",
"kms:Decrypt", "kms:DescribeKey"
"kms:Decrypt", "kms:DescribeKey", "kms:Encrypt",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like the reasons we add each of these permissions to be documented somewhere in-line. Can you think of a good place? (Unfortunately JSON doesn't allow comments... Sids?)

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 5, 2020

My theory is that since the pipeline account is defined as principal that is allowed to access the key in here the Cloudformation deployment action has implicit access to the key

The policy you quoted gives the role access to the key VIA S3, but the failing call in CloudTrail said the call was performed directly by CodePipeline (hence the policy denied it). No idea why that would be different. Maybe that's because there's a cross-account bucket access involved?

@NetaNir NetaNir added the pr/do-not-merge This PR should not be merged at this time. label Mar 6, 2020
@NetaNir NetaNir changed the title fix(cli): codepipeline cloudformation action in cross account fail… fix(cli): codepipeline cross account action fails writing output artifacts Mar 6, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8062aae
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@NetaNir NetaNir assigned NetaNir and unassigned shivlaks Mar 6, 2020
@rix0rrr rix0rrr merged commit 05cf78b into master Mar 9, 2020
@rix0rrr rix0rrr deleted the neta/fix-bootstrap-cross-account branch March 9, 2020 13:09
eladb pushed a commit that referenced this pull request Mar 9, 2020
…iting outputArtifacts (#6594)

To allow cross account actions in codepipeline we create a role in the other account which is assumable by codepipeline. Since the artifacts are stored in an encrypted S3 bucket in the code pipeline account, the role in the other account must have permission to access the bucket and KMS key in the codepipeline account. To give the role permissions to access the bucket and key two type of policies are required:

1. The policy defined on the bucket and key must give the other account permissions to perform all required actions.
2. The role policy must allow all required actions

Policy **1** is defined by the codepipeline construct. 
The role in the other account is created via the (new) bootstrap command and is defined as `DeploymentActionRole` in the `bootstrap-template.json` file. To satisfy **2**, the policy attached to the role must allow the required S3 and KMS actions. The policy attached to the role  was missing the required KMS actions to allow writing the output artifacts to the S3 bucket. This commits adds `kms:Encrypt`, `kms:ReEncrypt `, `kms:GenerateDataKey ` to the `DeploymentActionRole` attached policy
horsmand pushed a commit to horsmand/aws-cdk that referenced this pull request Mar 9, 2020
…iting outputArtifacts (aws#6594)

To allow cross account actions in codepipeline we create a role in the other account which is assumable by codepipeline. Since the artifacts are stored in an encrypted S3 bucket in the code pipeline account, the role in the other account must have permission to access the bucket and KMS key in the codepipeline account. To give the role permissions to access the bucket and key two type of policies are required:

1. The policy defined on the bucket and key must give the other account permissions to perform all required actions.
2. The role policy must allow all required actions

Policy **1** is defined by the codepipeline construct. 
The role in the other account is created via the (new) bootstrap command and is defined as `DeploymentActionRole` in the `bootstrap-template.json` file. To satisfy **2**, the policy attached to the role must allow the required S3 and KMS actions. The policy attached to the role  was missing the required KMS actions to allow writing the output artifacts to the S3 bucket. This commits adds `kms:Encrypt`, `kms:ReEncrypt `, `kms:GenerateDataKey ` to the `DeploymentActionRole` attached policy
rix0rrr pushed a commit that referenced this pull request Mar 11, 2020
…iting outputArtifacts (#6594)

To allow cross account actions in codepipeline we create a role in the other account which is assumable by codepipeline. Since the artifacts are stored in an encrypted S3 bucket in the code pipeline account, the role in the other account must have permission to access the bucket and KMS key in the codepipeline account. To give the role permissions to access the bucket and key two type of policies are required:

1. The policy defined on the bucket and key must give the other account permissions to perform all required actions.
2. The role policy must allow all required actions

Policy **1** is defined by the codepipeline construct. 
The role in the other account is created via the (new) bootstrap command and is defined as `DeploymentActionRole` in the `bootstrap-template.json` file. To satisfy **2**, the policy attached to the role must allow the required S3 and KMS actions. The policy attached to the role  was missing the required KMS actions to allow writing the output artifacts to the S3 bucket. This commits adds `kms:Encrypt`, `kms:ReEncrypt `, `kms:GenerateDataKey ` to the `DeploymentActionRole` attached policy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants