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(codebuild): Secret env variable from another account fails on Key decryption #14226

Merged
merged 4 commits into from
Apr 20, 2021

Conversation

Kruspe
Copy link
Contributor

@Kruspe Kruspe commented Apr 16, 2021

When providing a secretArn for the EnvironmentVariables allow kms:Decrypt
action for any key so that CodeBuild is able to get secrets that are stored
in different accounts.

fixes #14043


@skinny85 I didn't implement the if statement we talked about to check weither the secret actually lives in another account, since I wasn't able to come up with a good test for that. Any suggestions on this?

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

@gitpod-io
Copy link

gitpod-io bot commented Apr 16, 2021

@github-actions github-actions bot added the @aws-cdk/aws-codebuild Related to AWS CodeBuild label Apr 16, 2021
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Apr 16, 2021
@skinny85
Copy link
Contributor

Thanks for the contribution @Kruspe!

@skinny85 I didn't implement the if statement we talked about to check weither the secret actually lives in another account, since I wasn't able to come up with a good test for that. Any suggestions on this?

It's actually very easy to write tests in the CDK for cross-account usecases - they look exactly like other unit tests, you just provide different account numbers when creating the Stacks. Here's a simple example from the CodePipeline tests:

'adds a dependency on the Stack containing a new action Role'(test: Test) {
const region = 'us-west-2';
const pipelineAccount = '123456789012';
const buildAccount = '901234567890';
const app = new App();
const buildStack = new Stack(app, 'BuildStack', {
env: { account: buildAccount, region },
});
const actionRolePhysicalName = 'ProjectRolePhysicalName';
const actionRoleInOtherAccount = new iam.Role(buildStack, 'ProjectRole', {
assumedBy: new iam.AccountPrincipal(pipelineAccount),
roleName: actionRolePhysicalName,
});
const projectPhysicalName = 'ProjectPhysicalName';
const project = codebuild.Project.fromProjectName(buildStack, 'Project',
projectPhysicalName);
const pipelineStack = new Stack(app, 'PipelineStack', {
env: { account: pipelineAccount, region },
});
const bucket = new s3.Bucket(pipelineStack, 'ArtifactBucket', {
bucketName: 'source-bucket',
encryption: s3.BucketEncryption.KMS,
});
const sourceOutput = new codepipeline.Artifact();
new codepipeline.Pipeline(pipelineStack, 'Pipeline', {
artifactBucket: bucket,
stages: [
{
stageName: 'Source',
actions: [
new cpactions.S3SourceAction({
actionName: 'S3',
bucket,
bucketKey: 'path/to/file.zip',
output: sourceOutput,
}),
],
},
{
stageName: 'Build',
actions: [
new cpactions.CodeBuildAction({
actionName: 'CodeBuild',
project,
input: sourceOutput,
role: actionRoleInOtherAccount,
}),
],
},
],
});
expect(pipelineStack).to(haveResourceLike('AWS::CodePipeline::Pipeline', {
'Stages': [
{
'Name': 'Source',
},
{
'Name': 'Build',
'Actions': [
{
'Name': 'CodeBuild',
'Configuration': {
'ProjectName': projectPhysicalName,
},
'RoleArn': {
'Fn::Join': ['', [
'arn:',
{ 'Ref': 'AWS::Partition' },
`:iam::${buildAccount}:role/${actionRolePhysicalName}`,
]],
},
},
],
},
],
}));
test.equal(pipelineStack.dependencies.length, 1);
test.done();
},

@Kruspe Kruspe force-pushed the master branch 2 times, most recently from 226792c to 3854465 Compare April 18, 2021 00:04
@Kruspe
Copy link
Contributor Author

Kruspe commented Apr 18, 2021

@skinny85 thanks for the hint! Should be ready for review now.
I do not think this needs a README change. Let me know if you think otherwise.

When providing a secretArn from a another account for the EnvironmentVariables
allow kms:Decrypt action for any key. This enables CodeBuild to get the secret
without any further policy changes by the user.

fixes aws#14043
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks great @Kruspe! Couple of minor changes before we merge this in. Thanks for the contribution!

packages/@aws-cdk/aws-codebuild/test/test.project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/test/test.project.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codebuild/test/test.project.ts Outdated Show resolved Hide resolved
Kruspe added 2 commits April 20, 2021 09:56
Use Token.compareStrings to be able to compare the accounts in a better way and
handle tokens.
When providing two different secrets from another account we only want to
add the kms:Decrypt option for one resource since it already contains a wildcard
@mergify mergify bot dismissed skinny85’s stale review April 20, 2021 08:10

Pull request has been modified.

@Kruspe
Copy link
Contributor Author

Kruspe commented Apr 20, 2021

Alright. Thanks for the suggestions. I think all of them are valid and they are now implemented.

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks awesome @Kruspe, thanks so much for the contribution!

@skinny85 skinny85 changed the title feat(CodeBuild): add kms:Decrypt when secretArn is provided fix(codebuild): Secret en variable from another account fails on Key decryption Apr 20, 2021
@skinny85 skinny85 changed the title fix(codebuild): Secret en variable from another account fails on Key decryption fix(codebuild): Secret env variable from another account fails on Key decryption Apr 20, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 642fbcd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 8214338 into aws:master Apr 20, 2021
john-tipper pushed a commit to john-tipper/aws-cdk that referenced this pull request May 10, 2021
… decryption (aws#14226)

When providing a secretArn for the EnvironmentVariables allow kms:Decrypt
action for any key so that CodeBuild is able to get secrets that are stored
in different accounts.

fixes aws#14043

----

@skinny85 I didn't implement the if statement we talked about to check weither the secret actually lives in another account, since I wasn't able to come up with a good test for that. Any suggestions on this?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
… decryption (aws#14226)

When providing a secretArn for the EnvironmentVariables allow kms:Decrypt
action for any key so that CodeBuild is able to get secrets that are stored
in different accounts.

fixes aws#14043

----

@skinny85 I didn't implement the if statement we talked about to check weither the secret actually lives in another account, since I wasn't able to come up with a good test for that. Any suggestions on this?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-codebuild Related to AWS CodeBuild @aws-cdk/aws-kms Related to AWS Key Management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(CodeBuild): Add KMS decrypt to policy for secrets from another account
4 participants