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

pip preserves extracted file permissions with .zip but not .tar #1133

Closed
dtroyer opened this Issue Aug 12, 2013 · 20 comments

Comments

Projects
None yet
4 participants
@dtroyer

dtroyer commented Aug 12, 2013

Starting with pip 1.4 the file permissions inside .zip archives are preserved when extracted, this does not occur with .tar.* archives. The result is tar archive file permissions are controlled by the current umask where zip file archive permissions are literally preserved.

The problem observed is with packages that have files with 0600 permissions and are installed as root to the system path (non-venv). These files are readable only by root and any non-root process scanning sys.path now breaks with "IOError: [Errno 13] Permission denied: ...". The files extracted from tar archives continue to work as before as their permissions are worl-readable if the umask in place at the time of installation allowed it.

Packages with this feature include prettytable 0.7.2, that's a good test right there.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 12, 2013

Member

I think it's arguable that preserving the permissions of the archive is the wrong thing to do. These aren't normal archives they have a specific purpose, people are generally bad at packaging things, and it's unreasonable to expect there to even be a single set of permissions that make sense in every installation situation.

I believe that ensuring that the files end up being created with respect to the umask and not the permissions of the archive is the sane thing to do here. However either way we should be consistent, either use the permissions in the archive or set permissions based on umask.

Member

dstufft commented Aug 12, 2013

I think it's arguable that preserving the permissions of the archive is the wrong thing to do. These aren't normal archives they have a specific purpose, people are generally bad at packaging things, and it's unreasonable to expect there to even be a single set of permissions that make sense in every installation situation.

I believe that ensuring that the files end up being created with respect to the umask and not the permissions of the archive is the sane thing to do here. However either way we should be consistent, either use the permissions in the archive or set permissions based on umask.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 14, 2013

Contributor

this change came in with the merge of the wheel branch from @dholth : aa5b33d

I later updated it with a guard here: 7433867

I think this change was mirrored after the same change in distribute/setuptools
https://bitbucket.org/pypa/setuptools/diff/setuptools/archive_util.py?diff2=5b186f99d5e0&at=distribute

here's the distribute pull:
https://bitbucket.org/tarek/distribute/pull-request/14/set-permissions-when-extracting-from/diff
this pull claimed to be making the zip behavior consistent with the tar behavior.

I would need to look closer to be sure, but it does seem we should remove the new chmod that was added.

@dholth , @jaraco... help with the history on this?

Contributor

qwcode commented Aug 14, 2013

this change came in with the merge of the wheel branch from @dholth : aa5b33d

I later updated it with a guard here: 7433867

I think this change was mirrored after the same change in distribute/setuptools
https://bitbucket.org/pypa/setuptools/diff/setuptools/archive_util.py?diff2=5b186f99d5e0&at=distribute

here's the distribute pull:
https://bitbucket.org/tarek/distribute/pull-request/14/set-permissions-when-extracting-from/diff
this pull claimed to be making the zip behavior consistent with the tar behavior.

I would need to look closer to be sure, but it does seem we should remove the new chmod that was added.

@dholth , @jaraco... help with the history on this?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 14, 2013

Contributor

I do notice that setuptools' tar routine uses _extract_member for regular files, whereas pip uses extractfile. extract_member appears to preserve permissions, so the claim in the distribute pull would have been correct.

Contributor

qwcode commented Aug 14, 2013

I do notice that setuptools' tar routine uses _extract_member for regular files, whereas pip uses extractfile. extract_member appears to preserve permissions, so the claim in the distribute pull would have been correct.

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Aug 14, 2013

Member

Thanks for the analysis and digging up the relevant changes. I don't have any history to add other than the pull request already referenced. I also was a little skeptical of the assumption that preserving the permissions was the proper thing to do as @dstufft has expressed, but there was no objection. Let's figure out the right thing to do and then have a consistent approach for both.

Member

jaraco commented Aug 14, 2013

Thanks for the analysis and digging up the relevant changes. I don't have any history to add other than the pull request already referenced. I also was a little skeptical of the assumption that preserving the permissions was the proper thing to do as @dstufft has expressed, but there was no objection. Let's figure out the right thing to do and then have a consistent approach for both.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 15, 2013

Contributor

I'm thinking both setuptools and pip should not preserve permissions for tars or zips.

The argument in the distribute pull above was the need to "include non-Python executables in a Python distribution, which one must do from time to time." In my experience, including non-python shell scripts using the "scripts" keyword just works and they become executable, so I don't follow (unless he wanted them executable w/o marking them as "scripts"; but in that case, it's not really our problem).

It is true that the distutils docs says "Scripts are files containing Python source code", so it is confusing that it works for non-python scripts.

btw @dtroyer , I confirmed the 600 permissions using prettytable 0.7.2 from zip. Oddly enough the py files are fine (644), but the egg metadata is what's 600.

note #317 though which has people cheering for pip to preserve permissions.

Contributor

qwcode commented Aug 15, 2013

I'm thinking both setuptools and pip should not preserve permissions for tars or zips.

The argument in the distribute pull above was the need to "include non-Python executables in a Python distribution, which one must do from time to time." In my experience, including non-python shell scripts using the "scripts" keyword just works and they become executable, so I don't follow (unless he wanted them executable w/o marking them as "scripts"; but in that case, it's not really our problem).

It is true that the distutils docs says "Scripts are files containing Python source code", so it is confusing that it works for non-python scripts.

btw @dtroyer , I confirmed the 600 permissions using prettytable 0.7.2 from zip. Oddly enough the py files are fine (644), but the egg metadata is what's 600.

note #317 though which has people cheering for pip to preserve permissions.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 15, 2013

Member

So #317 deals specifically with executable files it appears. So we might want to special case execute permissions so that we preserve them (although it raises the question do we do them for just the current user, user and group, or user and group and world).

Member

dstufft commented Aug 15, 2013

So #317 deals specifically with executable files it appears. So we might want to special case execute permissions so that we preserve them (although it raises the question do we do them for just the current user, user and group, or user and group and world).

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 15, 2013

Contributor

the initial example in #317 is ctypes, which involves an elaborate extension. I wonder if the issue is it not being configured properly the way distutils recommends (or maybe the configure script just needs to marked as a "script" and used from the install location), and easy_install has just been forgiving all along.

special casing execute permissions sounds reasonable, maybe just for the current user.

Contributor

qwcode commented Aug 15, 2013

the initial example in #317 is ctypes, which involves an elaborate extension. I wonder if the issue is it not being configured properly the way distutils recommends (or maybe the configure script just needs to marked as a "script" and used from the install location), and easy_install has just been forgiving all along.

special casing execute permissions sounds reasonable, maybe just for the current user.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 15, 2013

Member

Actually it probably makes sense to just chmod +x it. If a user doesn't have read permissions they won't be able to execute the script either way. So it's still effectively controlled by the read/write permissions and we just treat the executable bit as something that affects world.

Member

dstufft commented Aug 15, 2013

Actually it probably makes sense to just chmod +x it. If a user doesn't have read permissions they won't be able to execute the script either way. So it's still effectively controlled by the read/write permissions and we just treat the executable bit as something that affects world.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 15, 2013

Member

Just spoke with @dtroyer on IRC and he concurs with this idea of just making anything with an execute bit world executable, but using the r/w permissions from umask.

Member

dstufft commented Aug 15, 2013

Just spoke with @dtroyer on IRC and he concurs with this idea of just making anything with an execute bit world executable, but using the r/w permissions from umask.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 15, 2013

Contributor

I don't follow automatically running chmod +x (world execution upgrade), if there's any executable permissions at all?

Contributor

qwcode commented Aug 15, 2013

I don't follow automatically running chmod +x (world execution upgrade), if there's any executable permissions at all?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 15, 2013

Contributor

ok, I think it makes sense, for global installs, that's what should be done.

Contributor

qwcode commented Aug 15, 2013

ok, I think it makes sense, for global installs, that's what should be done.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 15, 2013

Contributor

ok, so 1.4.2? and I'm willing to do this, unless you're on it.

Contributor

qwcode commented Aug 15, 2013

ok, so 1.4.2? and I'm willing to do this, unless you're on it.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 15, 2013

Member

Yes.

Essentially the read/write flags make sense to pull from umask because they will be the same for every file. However the problem then comes down to how do we let a package signal to us that it is executable. We could just respect whatever executable permissions are in the package, but that will break if the packager only expected user installs (700) and the installer is trying to make a world usable install (777). So we need to have some sort of way to detect what is an executable file, and some way to apply those to the newly created files. I think it's reasonable to say that any use of the execute bit makes it world executable.

One possible thing to do is to add an option for what umask you want executable files to use like PIP_EXECUTABLE_UMASK or something that would still give control to people who want it and could default to world executable + whatever the regular umask is. Not sure if it makes sense to bother with making that optional though.

Member

dstufft commented Aug 15, 2013

Yes.

Essentially the read/write flags make sense to pull from umask because they will be the same for every file. However the problem then comes down to how do we let a package signal to us that it is executable. We could just respect whatever executable permissions are in the package, but that will break if the packager only expected user installs (700) and the installer is trying to make a world usable install (777). So we need to have some sort of way to detect what is an executable file, and some way to apply those to the newly created files. I think it's reasonable to say that any use of the execute bit makes it world executable.

One possible thing to do is to add an option for what umask you want executable files to use like PIP_EXECUTABLE_UMASK or something that would still give control to people who want it and could default to world executable + whatever the regular umask is. Not sure if it makes sense to bother with making that optional though.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Aug 15, 2013

Member

I probably won't have time till maybe this weekend but likely not till next week to work on this.

Member

dstufft commented Aug 15, 2013

I probably won't have time till maybe this weekend but likely not till next week to work on this.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 15, 2013

Contributor

fwiw, I checked easy_install'ing prettytable 0.7.2 from zip too. same problem with the egg-info files being 600

Contributor

qwcode commented Aug 15, 2013

fwiw, I checked easy_install'ing prettytable 0.7.2 from zip too. same problem with the egg-info files being 600

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 16, 2013

Contributor

fyi, I'm working on this now. will need to backfill tests, which we don't have.

Contributor

qwcode commented Aug 16, 2013

fyi, I'm working on this now. will need to backfill tests, which we don't have.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 19, 2013

Contributor

@dtroyer feel free to try the branch in PR #1146 . I confirmed it installs prettytable from zip as expected, with the egg-info permissions being based on the umask, and not 600.

Contributor

qwcode commented Aug 19, 2013

@dtroyer feel free to try the branch in PR #1146 . I confirmed it installs prettytable from zip as expected, with the egg-info permissions being based on the umask, and not 600.

@dtroyer

This comment has been minimized.

Show comment
Hide comment
@dtroyer

dtroyer Aug 19, 2013

This looks good to me; I haven't run in to the executable permissions yet, but the 0600 bits are handled now. Thanks.

dtroyer commented Aug 19, 2013

This looks good to me; I haven't run in to the executable permissions yet, but the 0600 bits are handled now. Thanks.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Aug 20, 2013

Contributor

#1146 merged. closing. set to be released in 1.4.2

Contributor

qwcode commented Aug 20, 2013

#1146 merged. closing. set to be released in 1.4.2

@qwcode qwcode closed this Aug 20, 2013

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Aug 25, 2013

Member

I've created Setuptools #70 to address this same issue in Setuptools to match the pip behavior in easy_install.

Member

jaraco commented Aug 25, 2013

I've created Setuptools #70 to address this same issue in Setuptools to match the pip behavior in easy_install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment