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

Revert "Revert "fix: Respect zipped symlinks (#1140)"" #1482

Merged
merged 3 commits into from Apr 15, 2020

Conversation

jfuss
Copy link
Contributor

@jfuss jfuss commented Oct 25, 2019

This reverts commit 360d90a.

This commit originally reverted #1140 due to os.symlink not being
available on windows in Python2.7 stdlib, details here: #1315 (comment). We recently removed Python2.7 support in #1416, so this commit
is a revert of the revert, with some additional black formating to make make pr
pass.

Issue #, if available:

Description of changes:

Checklist:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mhart
Copy link
Contributor

mhart commented Oct 25, 2019

Will this also fix #477 ?

@jfuss
Copy link
Contributor Author

jfuss commented Oct 25, 2019

@mhart

Will this also fix #477 ?

No. This is only for unzipping and we are still relying on aws cloudformation package until #1437 is done. Then we will have full control over fixing issues like #477

Copy link
Member

@awood45 awood45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider addressing the comment above, but LGTM

@@ -77,7 +138,7 @@ def _set_permissions(zip_file_info, extracted_path):
"""

# Permission information is stored in first two bytes.
permission = zip_file_info.external_attr >> 16
permission = (zip_file_info.external_attr >> 16) & 511
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consider adding a comment explaining the & 511 part especially? May be useful to note for posterity/later development.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since @bubba-h57 wrote the initial pull request he might be able to give some background.

Copy link
Member

@awood45 awood45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to step back and make sure we understand what the & 511 line does to permissions before we can merge.

@awood45
Copy link
Member

awood45 commented Oct 29, 2019

Current plan is to add integ tests before merging.

@bubba-h57
Copy link
Contributor

zip_file_info.external_attr contains the files "attributes" (see stat.h)

#define S_IFIFO  0010000  /* named pipe (fifo) */
#define S_IFCHR  0020000  /* character special */
#define S_IFDIR  0040000  /* directory */
#define S_IFBLK  0060000  /* block special */
#define S_IFREG  0100000  /* regular */
#define S_IFLNK  0120000  /* symbolic link */
#define S_IFSOCK 0140000  /* socket */
#define S_ISUID 0004000 /* set user id on execution */
#define S_ISGID 0002000 /* set group id on execution */
#define S_ISTXT 0001000 /* sticky bit */
#define S_IRWXU 0000700 /* RWX mask for owner */
#define S_IRUSR 0000400 /* R for owner */
#define S_IWUSR 0000200 /* W for owner */
#define S_IXUSR 0000100 /* X for owner */
#define S_IRWXG 0000070 /* RWX mask for group */
#define S_IRGRP 0000040 /* R for group */
#define S_IWGRP 0000020 /* W for group */
#define S_IXGRP 0000010 /* X for group */
#define S_IRWXO 0000007 /* RWX mask for other */
#define S_IROTH 0000004 /* R for other */
#define S_IWOTH 0000002 /* W for other */
#define S_IXOTH 0000001 /* X for other */
#define S_ISVTX 0001000 /* save swapped text even after use */

We shift it right to drop off the unnecessary bits (pun included for no charge) leaving naught but the permissions we are interested in setting and then we then do a bitwise AND operation with 511.

Note that chmod 511 * would set the file permissions to -r-x--x--x? The bitwise AND guarantees that the resulting file permissions will always, at a minimum, be -r-x--x--x.

This is necessary because we really have no idea what sort of umask the developer is using, nor even what operating system (Windows? OSX? *NIX) so any file permission schema may have been used.

However, when run on AWS Lambda, the owner must have read access to all files.

We also don't know which files need to be executable, so we simply assume all files need to be executable.

As for testing, the various test environments can have some strange accounts/permissions setup. So we go ahead and ensure anyon can execute any file. Why? Because in the testing environments, it isn't always clear who the owner/executor is. Group/Other will only have read/write access if that is what the development version of the files has.

So, that's what is going on. Take the permission on the development file, overlay that with 511 permissions, and we guarantee that the file will be properly accessed and work in either testing or production environments.

@mhart
Copy link
Contributor

mhart commented Mar 11, 2020

Any news here? It seems that layers with symlinks still don't work with sam cli?

This reverts commit 360d90a.

This commit originally reverted aws#1140 due to os.symlink not being
available on windows in Python2.7 stdlib, details here: aws#1315 (comment). We recently removed Python2.7 support in aws#1416, so this commit
is a revert of the revert, with some additional black formating to make `make pr`
pass.
@jfuss jfuss force-pushed the symlink-zip-revert-revert branch from 513d9bb to f796a57 Compare April 15, 2020 21:03
@jfuss jfuss merged commit 9b2b0d5 into aws:develop Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants