Use default ACL for uploaded lambda code

cloudtools / stacker

An AWS CloudFormation Stack orchestrator/manager.

http://stacker.readthedocs.io/en/stable/

BSD 2-Clause "Simplified" License

711 stars 167 forks source link

Use default ACL for uploaded lambda code #682

Closed zollman closed 5 years ago

zollman commented 5 years ago

The "Authenticated-Read" ACL, currently set on all uploads, allows your code to be read by all S3 users. Default behavior should be to use the permissions implied by the bucket policy, i.e. "private".

Organizations that do not grant SetObjectAcl permissions (for fear of data loss) will block this call.

zollman commented 5 years ago

... getting flake8: error: no such option: --require-code make: *** [lint] Error 2

Running python 3.7, latest macOS

troyready commented 5 years ago

Seems great to me.

A strict semver interpretation might call this a 2.0 change. Alternatively, maybe no one ever interacted with the zipfiles outside of stacker and this could be in 1.x?

zollman commented 5 years ago

Given where this lives deep in the code, I could alternately make this a config or environment option & not change the default. My problem is that right now I have to monkeypatch to get the code to work, since I don't have SetObjectAcl permissions and therefore I can't use this code at all.

phobologic commented 5 years ago

This seems like a good change to me, though we probably should make it configurable (and default to the more secure option) just in case people do use this for some reason outside of stacker. Can you make those changes, @zollman ?

zollman commented 5 years ago

Will take me a few days, but sounds good @phobologic .

zollman commented 5 years ago

Added a config option. And just in time! AWS' new Block Public S3 buckets feature prevents authenticated-read also, leading to Access Denied errors: https://aws.amazon.com/blogs/aws/amazon-s3-block-public-access-another-layer-of-protection-for-your-accounts-and-buckets/

Caveats:

If you change payload_acl from one run to a second, it does not re-upload the file and fix the ACL - it skips because the file already exists
I deliberately did not write docs or checks for the set of canned ACLs, because AWS may change them. If you set an invalid payload_acl, you get an error on upload.

Let me know if this works.

Thanks for the guidance @phobologic & team - if you're at re:invent this week, I would love to say thanks in person.

phobologic commented 5 years ago

This is great, thanks @zollman - sorry for the delay in taking another look, and sorry for missing you at re:Invent - this is the first year I didn't go though! I'm working on fixing the circleci issues with moto/yaml, and once that's merged I'll be merging this :)