fix(codebuild): Secret env variable as token from another account fails on Key decryption #14483

Kruspe · 2021-05-01T00:39:11Z

fixes #14477

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

gitpod-io · 2021-05-01T00:39:13Z

packages/@aws-cdk/aws-codebuild/test/test.project.ts

Kruspe · 2021-05-03T21:29:07Z

I implemented the fix for the test that shows that different account secrets parsed as tokens are not working at the moment due to missing kms policies.
I also enhanced a different test that shows that we now give policies for the same account that is not needed. Since the secret is still an unresolved token, comparing the two accounts won't work here. Do you have a good suggestions how we could solve this?

Also let me know if we still need to clarify things about the use case. 😄

Kruspe · 2021-05-05T22:12:50Z

Okay this is the approach I opted for. It is still missing some tests and more implementation for:

Secret.fromSecretNameV2
json keys support

The if statement straight up sucks but I was too tired to figure out a better one. Maybe you can give me a hint or I will look at it tomorrow again. 😄

Any concerns or hints from your side @skinny85 ?

skinny85

Thanks a lot for the effort @Kruspe! This looks great, just needs a little bit more polish 🙂.

Also, make sure to update the ReadMe of the CodeBuild module to explain that you can pass Secrets directly in the value property!

packages/@aws-cdk/aws-codebuild/lib/project.ts

Pull request has been modified.

packages/@aws-cdk/aws-codebuild/lib/project.ts

skinny85

Putting this in 'Request changes' to clear it from my ToDo list, @Kruspe please re-request my review (there's a button in the top-right of the PR window, next to my avatar) when you're ready with the last changes!

Pull request has been modified.

Kruspe · 2021-05-21T15:50:33Z

Hey @skinny85 sorry for taking a while to get going here again. It was a busy week. Hope I didn't take up to much capacity in your brain.
But now I implemented the ideas we had and removed other implementations we didn't need anymore. Let me know what you think 😃

skinny85

Looks great @Kruspe! A few suggestions before we merge this in.

packages/@aws-cdk/aws-secretsmanager/test/secret.test.ts

packages/@aws-cdk/core/lib/secret-value.ts

packages/@aws-cdk/core/test/secret-value.test.ts

packages/@aws-cdk/aws-codebuild/test/test.project.ts

Pull request has been modified.

Kruspe · 2021-05-23T23:55:48Z

Hey @skinny85 I fixed mostly everything you mentioned above. There is one open point which we need to discuss (see the review above).
I added the extra fields for versionId and versionStage as you mentioned and also added an exception when both were provided since the documentation says this is not allowed.

When creating a SecretValue for SecretsManager save the values of the secret seperatly. Add test cases for SecretValues from an imported Secret

Add direct tests for providing a SecretValue via `SecretValue.secretsManager()`. Remove duplicated code and refactor type checking for SecretValue

We have to check this right away since SecretValues might not be passed as token but rather gets transformed into a string

When creating a new secret or importing it through a stack in another account we need to pass the account to the SecretValue. This allows us to check if the account for where the Project lives is in deed a different one.

…', which is in .gitallowed.

Add test for a secretArn which is provided as a token and fails at this moment due to a missing kms policy

skinny85

Looks great @Kruspe!

skinny85 · 2021-06-09T21:36:16Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

+                  // Work around a bug in SecretsManager -
+                  // when the access is cross-environment,
+                  // Secret.secretArn returns a partial ARN!
+                  // So add a "*" at the end, so that the permissions work


Suggested change

// So add a "*" at the end, so that the permissions work

// So add a "-??????" at the end, so that the permissions work

mergify · 2021-06-09T21:39:36Z

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 · 2021-06-09T22:15:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 9a8bf82
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-06-09T22:16:20Z

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).

…ls on Key decryption (aws#14483) fixes aws#14477 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot assigned skinny85 May 1, 2021

github-actions bot added the @aws-cdk/aws-codebuild Related to AWS CodeBuild label May 1, 2021

Kruspe mentioned this pull request May 1, 2021

(CodeBuild): Add KMS decrypt to policy for secrets imported by name #14477

Closed

Kruspe commented May 1, 2021

View reviewed changes

packages/@aws-cdk/aws-codebuild/test/test.project.ts Outdated Show resolved Hide resolved

Kruspe commented May 1, 2021

View reviewed changes

packages/@aws-cdk/aws-codebuild/test/test.project.ts Show resolved Hide resolved

skinny85 reviewed May 1, 2021

View reviewed changes

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

packages/@aws-cdk/aws-codebuild/test/test.project.ts Show resolved Hide resolved

Kruspe force-pushed the master branch 4 times, most recently from 37acdca to fd1e221 Compare May 3, 2021 21:06

Kruspe force-pushed the master branch 2 times, most recently from 7749b67 to 56dbd21 Compare May 5, 2021 22:00

skinny85 previously requested changes May 6, 2021

View reviewed changes

Kruspe force-pushed the master branch from 56dbd21 to 5496539 Compare May 7, 2021 16:36

Kruspe force-pushed the master branch from e88d377 to d8dad50 Compare May 10, 2021 20:36

Kruspe commented May 10, 2021

View reviewed changes

packages/@aws-cdk/aws-codebuild/lib/project.ts Outdated Show resolved Hide resolved

skinny85 previously requested changes May 19, 2021

View reviewed changes

Kruspe force-pushed the master branch from 36a3a56 to b252f17 Compare May 21, 2021 15:37

Kruspe requested a review from skinny85 May 21, 2021 15:50

skinny85 previously requested changes May 21, 2021

View reviewed changes

Kruspe force-pushed the master branch from 338669b to 3ed90a9 Compare May 23, 2021 23:53

Kruspe force-pushed the master branch from c4d9fc7 to 5df69df Compare May 25, 2021 20:07

Kruspe and others added 19 commits June 7, 2021 11:19

Add SecretQualifier to handle secret value

8a10c3a

When creating a SecretValue for SecretsManager save the values of the secret seperatly. Add test cases for SecretValues from an imported Secret

Refactor complex assertion

82dca33

Refactor tests

0891d06

Get SecretValue information from SecretQualifier

51340b1

Add direct tests for providing a SecretValue via `SecretValue.secretsManager()`. Remove duplicated code and refactor type checking for SecretValue

Remove unnecessary tests and add docs

697ec78

Remove exception for SecretValue not from SecretsManager

bad8870

Validate SecretValues are not of type plain text

b6aaa4a

We have to check this right away since SecretValues might not be passed as token but rather gets transformed into a string

Fix formatting

3cd1490

Remove duplicated assertion and error

957cd46

Move SecretValue versionStage and versionId check

d3feb24

Make SecretValue backwards compatible

b65b9cd

Add account to SecretQualifier

fd1c617

When creating a new secret or importing it through a stack in another account we need to pass the account to the SecretValue. This allows us to check if the account for where the Project lives is in deed a different one.

Change the used account in tests from '901234567890' to '012345678912…

5458a4d

…', which is in .gitallowed.

Make the Token case work!

c035d18

Revert changes that are not needed anymore

162df86

Move comment and remove duplicated test

8aaa2ec

Add test case for missing resource name

e07d4b0

Add test for a Secret import via complete ARN from a different account

6e9fb92

Add test for a secretArn which is provided as a token and fails at this moment due to a missing kms policy

Remove showcase test

48b9872

Kruspe force-pushed the master branch from 95254bf to 48b9872 Compare June 7, 2021 09:19

Kruspe marked this pull request as ready for review June 7, 2021 22:16

Merge branch 'master' into master

cdc594a

Kruspe requested a review from skinny85 June 9, 2021 06:07

skinny85 approved these changes Jun 9, 2021

View reviewed changes

Merge branch 'master' into master

9a8bf82

mergify bot merged commit 91e80d7 into aws:master Jun 9, 2021

	// So add a "*" at the end, so that the permissions work
	// So add a "-??????" at the end, so that the permissions work

fix(codebuild): Secret env variable as token from another account fails on Key decryption #14483

fix(codebuild): Secret env variable as token from another account fails on Key decryption #14483

Uh oh!

Conversation

Kruspe commented May 1, 2021

Uh oh!

gitpod-io bot commented May 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kruspe commented May 3, 2021

Uh oh!

Kruspe commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Kruspe commented May 21, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kruspe commented May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

skinny85 Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jun 9, 2021

Uh oh!

aws-cdk-automation commented Jun 9, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jun 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Kruspe commented May 5, 2021 •

edited

Loading

Kruspe commented May 23, 2021 •

edited

Loading