Opened 4 years ago

Closed 4 years ago

#20481 closed defect (fixed)

Extract source tarballs using permissions from umask

Reported by: embray Owned by:
Priority: minor Milestone: sage-7.3
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 4fe154a (Commits) Commit: 4fe154afb48510d212407e5a6c6b560fb91076a5
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This is the lowest-hanging fruit in addressing the issues I raised in this comment.

It changes sage-uncompress-spkg to apply the umask to all files and directories extracted from tarballs.

This doesn't apply to zipfiles since they do not contain permission information, though in principle we could modify zipfiles to use the same policy. I don't think it matters much though.

Change History (13)

comment:1 Changed 4 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

Instead of ignoring permissions for dirs, would it be possible to simply apply the user's umask on everything which gets extracted (files and directories). This would mean changing tarinfo.mode to tarinfo.mode & CURRENT_UMASK.

comment:3 Changed 4 years ago by embray

That was my first thought actually. I'd be fine with that too--this was just infinitesimally simpler :)

comment:4 follow-up: Changed 4 years ago by embray

Perhaps one reason not to prefer that is that if the umask is set to something useless then then applying it would be no less "safe". As it is this is only "unsafe" in the context of moving the extracted directory to someplace like /tmp. But it seems more reliable not to rely on the user's umask.

comment:5 in reply to: ↑ 4 Changed 4 years ago by jdemeyer

Replying to embray:

Perhaps one reason not to prefer that is that if the umask is set to something useless then then applying it would be no less "safe".

I would say that the umask defines what is safe. It is the thing which determines which permissions the user wants.

comment:6 Changed 4 years ago by novoselt

0700 would break the cell server which uses a separate worker account and in general would not work for multiuser setup. Or am I missing the point? It also seems to me that using umask is just the right way for defaults in any case. Those who are not happy with umask can either adjust it or set some restrictive permission on a containing directory.

comment:7 Changed 4 years ago by jdemeyer

I agree that building with too restrictive permissions could give problems. Some installation scripts probably just copy the directory over without changing the permissions. So 0700 is certainly too restrictive for that to work.

comment:8 Changed 4 years ago by embray

Okay then, umask it is. I'll update this next week.

comment:9 Changed 4 years ago by git

  • Commit changed from d2d0d740f933468ba4d6cbfaba1488f3423d5679 to 4fe154afb48510d212407e5a6c6b560fb91076a5

Branch pushed to git repo; I updated commit sha1. New commits:

4fe154aModified the custom TarFile to simply apply the user's umask to all files and directories, regardless what it may be--consistent with the default behavior of 'tar'.

comment:10 Changed 4 years ago by embray

As discussed, now the customized TarFile subclass merely applies the user's umask to all extracted files.

comment:11 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:12 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-7.2 to sage-7.3
  • Status changed from needs_review to positive_review
  • Summary changed from Extract source tarballs using more restrictive permissions on directories to Extract source tarballs using permissions from umask

comment:13 Changed 4 years ago by vbraun

  • Branch changed from u/embray/uncompress-permissions to 4fe154afb48510d212407e5a6c6b560fb91076a5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.