AWS - deployment bucket policy for HTTPS only

[neverendingqs]

What did you implement

This PR adds a policy to the deployment S3 bucket to reject all non-HTTPS requests.

I believe no documentation updates are necessary, but if wrong am happy to be pointed to the right places that require updates.

Closes #5621

How can we verify it

service: simple-service
provider:
  name: aws
  runtime: nodejs10.x
functions:
  hello:
    handler: handler.hello

Once deployed:

• CloudFormation will include a ServerlessDeploymentBucketPolicy resource
• S3 bucket with logical ID ServerlessDeploymentBucket will have a policy attached that restricts non-HTTPS requests

Todos

Useful Scripts

• npm run test-ci --> Run all validation checks on proposed changes
• npm run lint-updated --> Lint all the updated files
• npm run lint:fix --> Automatically fix lint problems (if possible)
• npm run prettier-check-updated --> Check if updated files adhere to Prettier config
• npm run prettify-updated --> Prettify all the updated files

• Write and run all tests
• Write documentation
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

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

[neverendingqs]

Doing it this way should prevent this test from breaking if additional statements are added in the future (vs. to.deep.include() the entire bucket policy resource).

[pmuens]

Thanks for working on this @neverendingqs 👍 💪

This looks good and I really like this change. I tested it by deploying an a service via master, then switching to your branch and re-rdeploying (to test for breaking changes). Furthermore I tested a whole deploy, re-deploy and remove lifecycle solely with this PR. Everything works as expected.

The only concern I have is that this change adds another resource which means that users might reach the 200 resource limit sooner. While this might sound like an exaggeration it's sometimes a real problem.

What do you think @medikoo?

[medikoo]

What do you think @medikoo?

If the only way to achieve it, is via extra resource, and we really should have that rule, then I think we should accept that.

Thankfully it's just 1 resource, and it's not one that will add some unexpected noise to AWS console (as e.g. custom resource lambda does)

[neverendingqs]

Something I thought of last night: if someone already has this in serverless.yml but named differently, this could break because buckets can only have one policy at a time (need to confirm, but seems to make sense to me). In that scenario, would this be a breaking change?

[medikoo]

In that scenario, would this be a breaking change?

It's a good question. I think we can ignore that case, at least it's very easy to recover from it, and I think if it'll happens it'll be really rare.

I've also just realized that in dashboard plugin we have a safeguard that if turned on, complains if such policy is missing, and if I read correctly it also complains on Serverless S3 bucket.

@dschep do you think this improvement is safe towards that safeguard?

[neverendingqs]

Gotcha.

Other thoughts:

• I didn't test this - will users be able to add to the bucket policy if they wanted to? I'm only vaguely aware of how config merges work
• Should the name of the bucket policy resource go in naming.js?

[medikoo]

I didn't test this - will users be able to add to the bucket policy if they wanted to

Yes, user resources are deep merged into ones as generated by the framework (it actually raises other issues -> #6575, but that's different story)

Should the name of the bucket policy resource go in naming.js?

Yes, I think that's our convention

[neverendingqs]

Yes, I think that's our convention

Gotcha. Sorry I've not be able to get to this until maybe late today (EDT).

[pmuens]

Thanks for updating the PR @neverendingqs! 👍

Merging in another policy should still work as long as the user uses the same resource logical id to overwrite the one defined here.

Waiting on @dschep with his answer as to how this PR affects safeguards (and vice versa).

[neverendingqs]

Just wanted to follow-up. Thanks.

[dschep]

@medikoo the safeguard only enforces rules, it doesn't automatically make the service comply with them. This would make the serverless framework comply with the safegaurd if a user has it enabled. I think it's a good thing to add, but because of the resource, yeah it might be good for this to be an option (regardless of if it's on by default or not)

[pmuens]

Given @dschep comment above 🔝 we should be GTM here if nothing else prevents us from merging it into master.

[alvinypyim]

Could we change the partition of the ARN to use Ref: AWS::Partition? Otherwise, this cannot be used in aws-us-gov or aws-cn.

[medikoo]

Could we change the partition of the ARN to use Ref: AWS::Partition

@alvinypyim if it's non breaking change, that just broadens support, then definitely. PR's welcome!