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

Don't create a gazillion kms keys #3275

Closed
1 of 5 tasks
jellevandenhooff opened this issue Jul 10, 2019 · 6 comments
Closed
1 of 5 tasks

Don't create a gazillion kms keys #3275

jellevandenhooff opened this issue Jul 10, 2019 · 6 comments
Labels
needs-triage This issue or PR still needs to be triaged.

Comments

@jellevandenhooff
Copy link

Note: for support questions, please first reference our documentation, then use Stackoverflow. This repository's issues are intended for feature requests and bug reports.

  • I'm submitting a ...

    • 🪲 bug report
    • 🚀 feature request
    • 📚 construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    Things like codepipeline pipelines create a S3 bucket and corresponding KMS key per pipeline.

  • What is the expected behavior (or behavior of feature suggested)?
    Create fewer KMS keys.

  • What is the motivation / use case for changing the behavior or adding this feature?
    That is going to add up pretty quickly, and is also difficult to clean up afterwards since the KMS key is not deleted automatically.

  • Please tell us about your environment:

    • CDK CLI Version: 0.39.0
    • Module Version: 0.39.0
    • OS: all
    • Language: TypeScript
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. associated pull-request, stackoverflow, gitter, etc)

@jellevandenhooff jellevandenhooff added the needs-triage This issue or PR still needs to be triaged. label Jul 10, 2019
@skinny85
Copy link
Contributor

Hey @jellevandenhooff ,

if you don't want to have the default behavior of creating the keys, feel free to provide your own Bucket when creating the Pipeline:

new codepipeline.Pipeline(this, 'Pipeline', {
  artifactBucket: new s3.Bucket(this, 'Bucket'),
  // ...
});

Is this good enough to solve your problem?

Thanks,
Adam

@jellevandenhooff
Copy link
Author

jellevandenhooff commented Jul 11, 2019 via email

@skinny85
Copy link
Contributor

It's kind of a chain of related issues that leads to this final state. We want to provide a secure default, so we encrypt the Bucket. However, CloudFormation cannot remove a Bucket that has any contents inside; so, to save you from having to go to the CloudFormation console every time you attempt to remove the Bucket from your Stack, we orphan it instead. Now, because we set a Key for it, we shouldn't really remove the Key either (as you wouldn't be able to read any of the contents of the Bucket if the Key was gone). So, we have to orphan the Key as well.

And that's why you get a Bucket + Key per created Pipeline by default.

@bgdnlp
Copy link

bgdnlp commented Jul 11, 2019

Wouldn't SSE-S3 be a better default in the sense that it would be less surprising?

@skinny85
Copy link
Contributor

Unfortunately, SSE-S3 does not cover all scenarios - in particular, cross-account access. We feel like the cross-account use case is important enough for CodePipeline that the default setup should work for it.

I'm going to close this issue; @bgdnlp @jellevandenhooff if there's anything you'd like to add to the discussion, please comment here, and I'll reopen it.

Thanks,
Adam

@skinny85
Copy link
Contributor

Good news - we will add an Alias to the default KMS Key created with the Pipeline, which should make it easy to hunt them down in the console. See #3694

skinny85 added a commit that referenced this issue Sep 11, 2019
This changes the scaffolding stack logic for the cross-region CodePipelines to include a KMS key and alias as part of it, which are required if an action is simultaneously cross-region and cross-account. We also change to use the KMS key ID instead of the key ARN when rendering the ArtifactStores property.

We also add an alias to the default pipeline artifact bucket.

This required a bunch of changes to the KMS and S3 modules:

* Alias now implements IKey
* Added the keyId property to IKey
* Added removalPolicy property to Alias
* Granting permissions to a key works if the principal belongs to a stack that is a dependent of the key stack
* Allow specifying a key when importing a bucket

Fixes #52
Concerns #1584
Fixes #2517
Fixes #2569
Concerns #3275
Fixes #3138
Fixes #3388
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

3 participants