fix: change permissions on package universally#4462
Conversation
sriram-mv
force-pushed
the
sam_package_perms
branch
from
e344024 to
69f15a6
Compare
| local_path, | ||
| uploader, | ||
| extension, | ||
| zip_method=make_zip_with_lambda_permissions if resource_id in LAMBDA_LOCAL_RESOURCES else make_zip, |
There was a problem hiding this comment.
added a check on just lambda specific resources.
| else: | ||
| self.assertEqual(permission_bits, 0o100644) | ||
| else: | ||
| self.assertEqual(permission_bits, 0o100755) |
There was a problem hiding this comment.
I might be miss reading this but looks like if you are not on windows, all permissions are 755? Shouldn't there be a directory vs file check here too?
There was a problem hiding this comment.
I have kept the permissions as-is because of the previous change on windows increased the permissions overall, it did not matter if it was a directory or a file.
There was a problem hiding this comment.
ooo.. You did an if not, which I missed. I find the opposite to be more readable, if windows: blah else: everything else.
Up to you on updating but I at least understand it now. :)
| os.remove(zipfile_name) | ||
| test_file_creator.remove_all() | ||
|
|
||
| def test_make_zip_lambda_resources(self): |
There was a problem hiding this comment.
Can we have tests that verify each resource we are doing this to and another one for the unsupported ones? I would rather over test this to ensure no escalation for resources we don't intend too (now and the future).
At the same note, Can we have a test that also verifies local permissions are not escalated? The code looks correct to me but just want to have a good suite of tests surrounding this change.
There was a problem hiding this comment.
For every lambda resource: https://github.com/aws/aws-sam-cli/pull/4462/files#diff-756169fa4653144117b5d40b30eb21a08c4b4a19ab93d0029a0d69afa2e9a692R342 we test that we are only ever hitting make_zip_with_lambda_permissions
sriram-mv
force-pushed
the
sam_package_perms
branch
from
2fec0be to
e2f6fe4
Compare
- set permissions on directories to be 755 and files to be 644.
- Lambda specific resource zips alone adhere to https://aws.amazon.com/premiumsupport/knowledge-center/lambda-deployment-package-errors
- except for windows, this was escalated earlier in https://github.com/aws/aws-sam-cli/pull/2356/files deliberately.
sriram-mv
force-pushed
the
sam_package_perms
branch
from
614126f to
0ab6c54
Compare
This reverts commit 7585e11.
This reverts commit 7585e11.
3 participants
Which issue(s) does this change fix?
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.