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

Update bucket conf to default AES256 encryption. #5800

Merged
merged 2 commits into from Feb 13, 2019

Conversation

Projects
4 participants
@a-h
Copy link
Contributor

a-h commented Feb 6, 2019

What did you implement:

Closes #5797

How did you implement it:

Trivial change to the default bucket policy.

How can we verify it:

Trivial change. Creating a simple Hello World function, and checking that the deployment bucket creating has default server-side encryption and versioning enabled via the AWS console would do it.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens self-assigned this Feb 6, 2019

@pmuens pmuens added the pr/in-review label Feb 6, 2019

@pmuens pmuens added this to In progress in Serverless via automation Feb 6, 2019

@a-h

This comment has been minimized.

Copy link
Contributor Author

a-h commented Feb 6, 2019

The Node.js: 4.4 build failed, because it didn't recognise the spread operator, which I'd used to carry out Object.assign operations.

I could replace it with this?

    mergeProperties.reduce((acc, val) => Object.assign(acc, val),
      this.serverless.service.provider.compiledCloudFormationTemplate
        .Resources.ServerlessDeploymentBucket.Properties);

Serverless automation moved this from In progress to Needs review Feb 6, 2019

@pmuens
Copy link
Member

pmuens left a comment

This looks good! Thanks for jumping in and working on this @a-h 👍 👌

I tested it and it works without any issue. I found an issue in your code regarding Node.js 4 support (which we still have to support).

Before moving on I'd like to understand if this could negatively affect existing deployments? It looks like this will only kick-in if you deploy a new service since the changes are in the core template. Does this add any additional costs, limitations or maybe even downsides for (existing) users?

Thanks again for working on this @a-h 💯


Edit: Looks like removing a service causes some problems. After deploying a couple of times the removal process complains that there are some objects left in the bucket which need to be removed. Looks like this is caused by the usage of versioning.

Show resolved Hide resolved lib/plugins/aws/package/lib/generateCoreTemplate.js Outdated
@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Feb 6, 2019

Enabling Encryption is welcome but what is a motivation to enable versioning on deployment bucket?

If I understand correctly, versioning keep history and increase sizes for deployment bucket, therefore increase costs (thought it quite small).
IMO, I manage versions in my own git repo, so I do not understand merit of s3 versioning for deployment bucket.

Perhaps I miss some context?

@a-h

This comment has been minimized.

Copy link
Contributor Author

a-h commented Feb 6, 2019

Versioning protects against accidental (or malicious deletion, especially when combined with MFA delete) deletion of the content and provides an audit history of all the code that was deployed.

Ensuring that versioning is enabled is a default rule within AWS Config and other AWS audit tools (e.g. Scout2). So the aim is to make the default Serverless config as production-ready as it can be / not stick out in audits and require later remediation.

Since S3 versioning keeps all old versions, then there are costs associated with the storage. This could be remediated by including a lifecycle policy to remove the old versions after a default period (e.g. 1 year?).

As to whether it should be the default, versionFunctions is set to true by default (ran out of space a few times now :)), so I thought that it might make sense to be default here.

There are no additional charges for enabling default S3 configuration, so that's a relatively easy win.

Happy to drop versioning, or turn it into an option. Maybe I should split versioning into another issue / PR and debate that there?

@darrenhgc

This comment has been minimized.

Copy link

darrenhgc commented Feb 6, 2019

Happy to drop versioning, or turn it into an option. Maybe I should split versioning into another issue / PR and debate that there?

This. I believe this warrants more discussion.

@a-h a-h changed the title Update bucket conf to default AES256 encryption and enable versioning. Update bucket conf to default AES256 encryption. Feb 6, 2019

@pmuens

pmuens approved these changes Feb 7, 2019

Copy link
Member

pmuens left a comment

Thank you very much for addressing the issues @a-h 👍

I took it for another spin and multiple deployments, followed by a removal work just fine! Really happy to see that we now have S3 bucket encryption enabled by default! 👌

Back then we built in versioning since we always keep the 5 most recent deployments in the bucket.

I'd suggest that we open up a separate issue and get some feedback if versioning enabled on a bucket level would make sense. After we've heard enough feedback we could tackle it in a separate PR. 🤔

I have one last question regarding this change: It appeas that only the core template is changed here, meaning that only from-scratch deployments would get encrypted buckets, correct? So other services which are already out in the wild won't be updated and hence won't be affected by this change?

I'm almost 100% sure that the answer is yes, just double-checking so that we don't break existing deployments by accident...

Again, thanks for working on this @a-h 💯 🥇

Serverless automation moved this from Needs review to Reviewer approved Feb 7, 2019

@a-h

This comment has been minimized.

Copy link
Contributor Author

a-h commented Feb 7, 2019

Thanks for your enthusiastic comments, I feel very welcomed to the project!

Default encryption will be applied to existing buckets which were created by the Serverless Framework with this change. The encryption is transparent and uses Server-Side Encryption with Amazon S3-Managed Keys (SSE-S3), so there's no need to set a key, or permissions concerns as there would be might be with KMS-based encryption.

To test the effect on existing projects, I tested it on one of my projects which I'd previously deployed. The bucket had no encryption enabled and was using the default Serverless Framework configuration. After running the updated Serverless framework deployment, default encryption was then applied to the bucket, new objects added to the bucket (deployments) were encrypted, but objects that were already in the bucket (previous deployments) were not updated to be encrypted. I found no issues with the deployment or changes.

I've also tested it with a custom bucket configuration to make sure that custom configurations are not overwritten. I created an S3 bucket with versioning enabled, which uses a custom KMS key for encryption.

screenshot 2019-02-07 at 11 40 27

I then tested using a custom bucket instead of the default:

provider:
  name: aws
  runtime: nodejs8.10
  deploymentBucket:
    name: xxxx-serverless-deployments
  stage: dev
  region: eu-west-2

I deployed it once using the current version of Serverless framework, then again using the version with this pull request applied. Afterwards, no changes were made to my custom bucket, as expected.

@exoego

exoego approved these changes Feb 9, 2019

Copy link
Contributor

exoego left a comment

LGTM 👍
Thanks for understanding my concerns for versioning.

@pmuens

pmuens approved these changes Feb 13, 2019

Copy link
Member

pmuens left a comment

Hey @a-h thanks again for working on this and @exoego thanks for your review and assessment 👍 🥇

I just tested it thoroughly with an old service and did a couple of deployments and removals. Couldn't break it.

Great job @a-h 👌

Merging :shipit:

@pmuens pmuens merged commit 415ca07 into serverless:master Feb 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Feb 13, 2019

@pmuens pmuens added this to the 1.38.0 milestone Feb 13, 2019

@a-h a-h deleted the a-h:enable_s3_encryption branch Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.