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

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

Closed
Kruspe opened this issue Apr 30, 2021 · 9 comments · Fixed by #14483
Closed

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

Kruspe opened this issue Apr 30, 2021 · 9 comments · Fixed by #14483
Assignees
Labels
@aws-cdk/aws-codebuild Related to AWS CodeBuild effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@Kruspe
Copy link
Contributor

Kruspe commented Apr 30, 2021

Following up on #14043 and #14226. I was thinking about also allowing the kms:Decrypt action for secrets that get provided via Token.

if (envVariable.type === BuildEnvironmentVariableType.SECRETS_MANAGER) {
if (Token.isUnresolved(envVariableValue)) {
// the value of the property can be a complex string, separated by ':';
// see https://docs.aws.amazon.com/codebuild/latest/userguide/build-spec-ref.html#build-spec.env.secrets-manager
const secretArn = envVariableValue.split(':')[0];
// if we are passed a Token, we should assume it's the ARN of the Secret
// (as the name would not work anyway, because it would be the full name, which CodeBuild does not support)

Since we already assume that the value is an Arn, we could parse it and then create an Arn for the kms key with the wildcard and add it to the set of kmsIamResources.

If this is worth implementing I can create the PR. :)

Environment

  • CDK CLI Version : 1.101.0
  • Framework Version: 1.101.0
  • Node.js Version: 15.11.0
  • OS : MacOS
  • Language (Version): all

This is 🐛 Bug Report

@Kruspe Kruspe added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2021
@github-actions github-actions bot added the @aws-cdk/aws-codebuild Related to AWS CodeBuild label Apr 30, 2021
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Apr 30, 2021
@skinny85
Copy link
Contributor

Hey @Kruspe ,

why would the CodeBuild role need kms:Decrypt? Isn't that used only for writing using the Key, while the Project will only read using it (it will read the secrets from SecretsManager)?

Thanks,
Adam

@skinny85 skinny85 added feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed @aws-cdk/aws-kms Related to AWS Key Management bug This issue is a bug. labels Apr 30, 2021
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Apr 30, 2021
@Kruspe
Copy link
Contributor Author

Kruspe commented Apr 30, 2021

Hey @skinny85,

kms:Decrypt is used by SecretsManager when getting the secret. Have a look here

We already give that permission when the secret is added via Arn here

So it would be just a matter of adding the policy to the kmsIamResources Set.

@skinny85
Copy link
Contributor

Ah, OK. For some reason I thought we granted kms:Encrypt, but yeah, that doesn't make sense 😜.

@skinny85
Copy link
Contributor

If the value of the SecretsManager env variable is a Token, doesn't it mean it's referencing a Secret from the same account?

We only add the kms:Decrypt permissions when the Secret is from a different account.

@Kruspe
Copy link
Contributor Author

Kruspe commented Apr 30, 2021

I do not think so. The Ref can also point to a secret in another account. The policy for secrets manager already works just kms:decrypt is missing.
So I would say it makes sense to add the behavior also for tokens. I would like to implement the different account check and it is possible through Token.compareString though I am not sure how a test would look like. The secret we create would have to have a different account. I didn't check if we can create one in the test case.

Side note: Right now we add a resource for every token, meaning that tokens which are extended with :username but reference the same secret add the same arn to the resource. Maybe we can fix that as well ^^

@skinny85
Copy link
Contributor

I do not think so. The Ref can also point to a secret in another account.

Nope! That is not allowed in CloudFormation.

So I would say it makes sense to add the behavior also for tokens. I would like to implement the different account check and it is possible through Token.compareString though I am not sure how a test would look like. The secret we create would have to have a different account. I didn't check if we can create one in the test case.

Gotta say, I'm not following this at all.

Maybe you can create a draft Pull Request, and show me in a unit test what is the expected behavior?

Side note: Right now we add a resource for every token, meaning that tokens which are extended with :username but reference the same secret add the same arn to the resource. Maybe we can fix that as well ^^

Again, you lost me here. Another unit test in that draft PR?

@Kruspe
Copy link
Contributor Author

Kruspe commented May 1, 2021

Alright draft is ready at #14483. Sorry for the confusion. I hope this clarifies things. :)


Nope! That is not allowed in CloudFormation.

Does the PR clarify this for you?

Gotta say, I'm not following this at all.

Let's ignore the solution for now and focus on the use case I would say.

@skinny85
Copy link
Contributor

skinny85 commented May 1, 2021

Awesome, thanks!

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 1, 2021
Kruspe added a commit to Kruspe/aws-cdk that referenced this issue May 3, 2021
Kruspe added a commit to Kruspe/aws-cdk that referenced this issue May 3, 2021
Kruspe added a commit to Kruspe/aws-cdk that referenced this issue May 3, 2021
Kruspe added a commit to Kruspe/aws-cdk that referenced this issue May 3, 2021
Kruspe added a commit to Kruspe/aws-cdk that referenced this issue May 3, 2021
@njlynch njlynch removed their assignment May 14, 2021
@skinny85 skinny85 added effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p1 and removed needs-triage This issue or PR still needs to be triaged. @aws-cdk/aws-kms Related to AWS Key Management labels May 20, 2021
@mergify mergify bot closed this as completed in #14483 Jun 9, 2021
mergify bot pushed a commit that referenced this issue Jun 9, 2021
…ls on Key decryption (#14483)

fixes #14477


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jun 9, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
…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*
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 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
3 participants