Opened 11 years ago

Closed 8 years ago

#11676 closed enhancement (wontfix)

sage-pkg does not force world-readable permissions

Reported by: AlexanderDreyer Owned by: AlexanderDreyer
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: scripts Keywords: chmod umask install mode
Cc: jdemeyer, leif Merged in:
Authors: Reviewers: Alexander Dreyer, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

As pointed out here: #11660 and here: #11664 archives have to be world-readable. sage-pkg does not enforce this.

The following patch adds options to sage -pkg which allow to force anonymous or world-readable file settings (or both).

Apply sage-pkg-tarmode.2.patch

Attachments (3)

sage-pkg-tarmode.0.patch (936 bytes) - added by AlexanderDreyer 11 years ago.
Monkey patching permssions etc. for sage -pkg
sage-pkg-tarmode.1.patch (1.0 KB) - added by AlexanderDreyer 11 years ago.
beter Monkey patch (checks directory permissions)
sage-pkg-tarmode.2.patch (5.1 KB) - added by AlexanderDreyer 11 years ago.
Making enforcement optional, also fixing Darwin

Download all attachments as: .zip

Change History (38)

comment:1 Changed 11 years ago by leif

  • Cc leif added

comment:2 Changed 11 years ago by kini

What is the desired resolution? sage-pkg should not modify the src/ directory when making an SPKG.

comment:3 Changed 11 years ago by leif

  • Component changed from misc to scripts

I'd also say they should, but they don't have to. ;-)

It would be better to just change them in sage-spkg (including umask) if at all.

The proper way would IMHO be to fix the permissions of the installed files.

sage-pkg could issue a warning though.

comment:4 follow-up: Changed 11 years ago by leif

  • Keywords chmod umask install mode added

As mentioned elsewhere, if all files would get installed with install (and -m) this would be a non-issue.

We may patch distutils for Python packages accordingly.

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

Replying to leif:

As mentioned elsewhere, if all files would get installed with install (and -m) this would be a non-issue.

This is not going to happen, unless you want to patch all upstream packages.

I am personally in favour of a warning, instead of forcably changing permissions.

Changed 11 years ago by AlexanderDreyer

Monkey patching permssions etc. for sage -pkg

comment:6 Changed 11 years ago by AlexanderDreyer

  • Status changed from new to needs_review

comment:7 Changed 11 years ago by AlexanderDreyer

  • Authors set to Alexander Dreyer
  • Owner changed from jason to AlexanderDreyer

comment:8 follow-up: Changed 11 years ago by leif

Well, it's sometimes better to know who packaged an spkg (though one can easily fake this of course). ;-)

Changed 11 years ago by AlexanderDreyer

beter Monkey patch (checks directory permissions)

comment:9 Changed 11 years ago by AlexanderDreyer

  • Description modified (diff)

comment:10 Changed 11 years ago by AlexanderDreyer

  • Description modified (diff)

comment:11 Changed 11 years ago by jdemeyer

  • Report Upstream changed from None of the above - read trac for reasoning. to N/A

I'm still not convinced that this is the correct way to handle this. I dislike the idea of forcing certain permissions, I prefer a warning (and it seems leif also).

comment:12 in reply to: ↑ 8 ; follow-up: Changed 11 years ago by jdemeyer

  • Description modified (diff)

Replying to leif:

Well, it's sometimes better to know who packaged an spkg (though one can easily fake this of course). ;-)

I don't see the point why this information is useful, one would better look at the hg log or SPKG.txt for this. With my current merge-script this information is lost anyway because every spkg is unpacked and repacked (in between changes are committed, a hg tag is added and the spkg is checked for some obvious mistakes).

comment:13 in reply to: ↑ 12 Changed 11 years ago by leif

Replying to jdemeyer:

Replying to leif:

Well, it's sometimes better to know who packaged an spkg (though one can easily fake this of course). ;-)

I don't see the point why this information is useful, one would better look at the hg log or SPKG.txt for this. With my current merge-script this information is lost anyway because every spkg is unpacked and repacked (in between changes are committed, a hg tag is added and the spkg is checked for some obvious mistakes).

Yep, in the Sage distros.

I won't object making both modifications optional, i.e. adding separate options to sage-pkg for changing the owner/group, and the file modes ("permissions").

The first could default to "yes" (or True), the second IMHO shouldn't.

comment:14 Changed 11 years ago by leif

A warning should IMHO independently be given in case there are files with "suspicious" permissions, perhaps with an option to disable this, as there are usually a lot of files that don't get installed (or at least not by cp) where the permissions hardly matter.

comment:15 Changed 11 years ago by AlexanderDreyer

Just another 2pence from me: in my current environment permissions and owners/groups are sometimes enforced by the file system. So the only way to reliably change is within the tarball. So: should I prepare a patch which makes enforcing optional?

Changed 11 years ago by AlexanderDreyer

Making enforcement optional, also fixing Darwin

comment:16 Changed 11 years ago by AlexanderDreyer

  • Description modified (diff)
  • Owner changed from AlexanderDreyer to (none)

So here's an patch which makes things optional, for instance use sage -pkg -wa dir for all extensions. See sage -pkg -h for details.

comment:17 Changed 11 years ago by AlexanderDreyer

  • Type changed from defect to enhancement

comment:18 Changed 11 years ago by AlexanderDreyer

  • Description modified (diff)

comment:19 Changed 11 years ago by AlexanderDreyer

  • Owner changed from (none) to AlexanderDreyer

comment:20 follow-up: Changed 11 years ago by jdemeyer

  • Description modified (diff)

My personal opinion on this is still that we don't need any of these options, I don't think sage -spkg should mess with permissions or user names.

comment:21 Changed 11 years ago by kini

I agree. Certainly sage-spkg should not touch the src/ directory under any circumstances, IMO, as the SPKG format should be flexible enough to be distributed sans the src/ directory with the expectation that a particular upstream source archive can be extracted into src/ before running spkg-install (if we at some point decide to build a more complex package manager, etc.).

I don't think that changes to the other files in the SPKG should be made by sage-spkg immediately before archiving, either, because this results in uncommitted changes in the Mercurial repository shipped inside the SPKG, which is not good. Though I guess you may disagree with me on this point, Jeroen, as (IIRC) you use scripts which make automated commits in SPKGs before publishing them, right?

comment:22 in reply to: ↑ 20 ; follow-ups: Changed 11 years ago by AlexanderDreyer

Replying to jdemeyer:

My personal opinion on this is still that we don't need any of these options, I don't think sage -spkg should mess with permissions or user names.

First of all, this is a patch to sage-pkg, not sage-spkg. So I'm talking about generating an spkg, not installing it. But remember, the original reason for this patch was, that you indeed proposed in #11664 to change the src (namely to give world-readable permissions).

Since I'm forced to work on a non-standard network file system, where I cannot enforce the requested permissions, I have to change the permissions while bundling the spkg, i.e. when generating the tar-file. (Despite from my broken file system, the same is true, when working on a fat-based drive, may USB sticks, etc.)

So, in order to provide valid PolyBoRi spkgs in the future I have to apply this patch.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 11 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Replying to AlexanderDreyer:

Since I'm forced to work on a non-standard network file system, where I cannot enforce the requested permissions, I have to change the permissions while bundling the spkg, i.e. when generating the tar-file. (Despite from my broken file system, the same is true, when working on a fat-based drive, may USB sticks, etc.)

Partially off-topic but highly important question: what does your "broken" filesystem do to executable permissions? If it doesn't preserve permissions, it might also change the executable bit which is very bad anyway.

Could you please tell me which OS you are using, which filesystem it is precisely and what happens when you do

$ chmod 644 some_file
$ ls -l some_file
$ chmod 755 some_file
$ ls -l some_file

on that filesystem? I'm just trying to understand the situation better so I can find out the best fix.

comment:24 Changed 11 years ago by kini

Er, sorry, I meant sage -pkg rather than sage -spkg, as I assume Jeroen did too.

comment:25 in reply to: ↑ 22 Changed 11 years ago by jdemeyer

Replying to AlexanderDreyer:

First of all, this is a patch to sage-pkg, not sage-spkg. So I'm talking about generating an spkg, not installing it.

That's what I meant, though I might have said it wrongly.

comment:26 in reply to: ↑ 23 ; follow-up: Changed 11 years ago by AlexanderDreyer

Replying to jdemeyer:

Partially off-topic but highly important question: what does your "broken" filesystem do to executable permissions? If it doesn't preserve permissions, it might also change the executable bit which is very bad anyway.

The (owner) execetable bit is not affected.

Could you please tell me which OS you are using, which filesystem it is precisely and what happens when you do

$ chmod 644 some_file
$ ls -l some_file
$ chmod 755 some_file
$ ls -l some_file

on that filesystem? I'm just trying to understand the situation better so I can find out the best fix.

It is Linux, in particular SuSE Enterprise 11, but that's not part of the problem. The file are on an standalone file server which caches a lot of things. It seems there is a bug in the fileserver's system or in the driver, so that permissions are sometimes "stuck", i.e. chmod doesn't to anything (resp. the changes last some minutes to become active).

But anyway, as I understand from #11664 the sources must be world readable in any case. So Sage should provides a tool to ensure this while packaging - at least optionally. Alternatively, sage-pkg should test the permissions and give a warning if necessary.

BTW: For the second part of the patch - namely changing the user/group to root - this should be done for privacy reasons anyway. (This can only be done in the tar file)

comment:27 in reply to: ↑ 26 ; follow-up: Changed 11 years ago by leif

Replying to AlexanderDreyer:

But anyway, as I understand from #11664 the sources must be world readable in any case. So Sage should provides a tool to ensure this while packaging - at least optionally. Alternatively, sage-pkg should test the permissions and give a warning if necessary.

Well, in principle only files that are to be copied (with -p) into the Sage installation tree should have 0644 or 0755 permissions (one should usually use some BSD-like install where this isn't an issue at all), but it's of course nearly impossible to check that when doing an automatic "sanity" check on an spkg.


BTW: For the second part of the patch - namely changing the user/group to root - this should be done for privacy reasons anyway. (This can only be done in the tar file)

FWIW, doing

$ tar -C /tmp -xvjf foo-x.y.z.pN.spkg &&
  tar -C /tmp --owner=0 --group=0 -cvjf foo-x.y.z.pN.spkg foo-x.y.z.pN

you can achieve the same.

(I personally don't think privacy really matters here, especially since you can fake arbitrary user/group names or IDs, and your e-mail address should be contained in the log anyway. Instead, it is IMHO useful to see who packaged an spkg, modulo that there need not be a relation to a user you really know. Alternatively, you could also use numeric UIDs/GIDs for the tarball in case you feel using your real logname is dangerous from a security point of view.)

comment:28 in reply to: ↑ 27 ; follow-up: Changed 11 years ago by AlexanderDreyer

Replying to leif:

Well, in principle only files that are to be copied (with -p) into the Sage installation tree should have 0644 or 0755 permissions (one should usually use some BSD-like install where this isn't an issue at all), but it's of course nearly impossible to check that when doing an automatic "sanity" check on an spkg.

The installation tree of that old spkg had always been sane, since PolyBoRi enforces the permissions on install.

The permissions in the source tree were considered as buggy. But if you what specific permissions in the source, you need a tool to either enforce or check this. (That patch for checking would be trivial. I'll provide it, if there's a chance for a review.)

FWIW, doing

[...]

you can achieve the same.

Right, but others could be trapped in the same way.

(I personally don't think privacy really matters here, especially since you can fake arbitrary user/group names or IDs, and your e-mail address should be contained in the log anyway. Instead, it is IMHO useful to see who packaged an spkg, modulo that there need not be a relation to a user you really know. Alternatively, you could also use numeric UIDs/GIDs for the tarball in case you feel using your real logname is dangerous from a security point of view.)

My group id encodes part of my institute's organization and even details of my contract (if you know how to read it). If you want security you need to sign spkgs. But that's another quest.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 11 years ago by leif

Replying to AlexanderDreyer:

The permissions in the source tree were considered as buggy. But if you what specific permissions in the source, you need a tool to either enforce or check this. (That patch for checking would be trivial. I'll provide it, if there's a chance for a review.)

I'd rather set the umask in sage-spkg, and do a chmod -R +rX on the extracted spkg upon installation, if one wants to go triple-safe.

The added spkg "sanity check" regarding permissions is IMHO superfluous and rather annoying; as Keshav mentioned, we should in general leave the upstream alone, i.e., ship it really vanilla, and maybe fix individual permissions in the corresponding spkg-install file if necessary.


If you want security you need to sign spkgs. But that's another quest.

I meant security on your side, not on the user's who installs an spkg.

Signing spkgs, at least those officially shipped or made available, wouldn't be bad either, but that's a different issue. Of course also every Sage developer could sign his spkgs; I usually provide md5sums for mine, although for a different reason. But as far as I know Jeroen currently repackages all spkgs anyway before they get merged into a release, just like commit messages of patches get "beautified", regardless of whether they already contain the ticket number.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 11 years ago by AlexanderDreyer

  • Milestone changed from sage-4.7.2 to sage-duplicate/invalid/wontfix

Replying to leif:

The added spkg "sanity check" regarding permissions is IMHO superfluous and rather annoying; as Keshav mentioned, we should in general leave the upstream alone, i.e., ship it really vanilla, and maybe fix individual permissions in the corresponding spkg-install file if necessary.

I don't agree (it would have helped to avoid #11664), but it seems there's no way to fix the ticket.

comment:31 in reply to: ↑ 30 ; follow-up: Changed 11 years ago by leif

Replying to AlexanderDreyer:

Replying to leif:

The added spkg "sanity check" regarding permissions is IMHO superfluous and rather annoying; as Keshav mentioned, we should in general leave the upstream alone, i.e., ship it really vanilla, and maybe fix individual permissions in the corresponding spkg-install file if necessary.

I don't agree (it would have helped to avoid #11664), but it seems there's no way to fix the ticket.

I didn't mean sage -pkg (i.e., a sanity check regarding permissions there, although you'd have the same problems unless you're upstream), but the merger script which complains about any "suspicious" permissions since -- as mentioned -- it's hard or impossible to do a reasonable check regarding this, i.e., to make sure all installed files have or will have the correct permissions.

So I wouldn't mind if sage -pkg issued warnings (to perhaps be disabled by some option), but the proper or safest way to avoid such issues is IMHO still to set the umask and do a chmod -R on src/ at least (from sage-spkg), although this will in most cases be redundant or superfluous (e.g. if install -c -m ... is used by the spkg anyway, or all files to be copied with cp -p already have the proper permissions). In fact, a sanity check should be made on the Sage installation, after one or more (or all) spkgs have been built and installed.

comment:32 in reply to: ↑ 31 ; follow-up: Changed 11 years ago by AlexanderDreyer

Replying to leif:

So I wouldn't mind if sage -pkg issued warnings (to perhaps be disabled by some option), but the proper or safest way to avoid such issues is IMHO still to set the umask and do a chmod -R on src/ at least (from sage-spkg), although this will in most cases be redundant or superfluous (e.g. if install -c -m ... is used by the spkg anyway, or all files to be copied with cp -p already have the proper permissions). In fact, a sanity check should be made on the Sage installation, after one or more (or all) spkgs have been built and installed.

umask alone is not enough, the standalone file server, where my projects live, is configured to override the user's umask and enforces world-writable permissions. Installed files should not preserve them, so install (or something equivalent) is necessary indeed. chmod is not work always be effective. For instance, think of working on a FAT-based USB-drive. The latter has fixed permissions, set on mount time.

comment:33 in reply to: ↑ 32 Changed 11 years ago by jdemeyer

Replying to AlexanderDreyer:

Replying to leif: chmod is not work always be effective. For instance, think of working on a FAT-based USB-drive. The latter has fixed permissions, set on mount time.

As I mentioned above, that would be very bad anyway, because you also lose information on executable permissions. Linux, for example, treats all files on a FAT system as executable.

comment:34 Changed 8 years ago by jdemeyer

  • Authors Alexander Dreyer deleted
  • Reviewers set to Alexander Dreyer, Jeroen Demeyer
  • Status changed from needs_info to positive_review

Should be closed since sage-pkg is obsolete.

comment:35 Changed 8 years ago by vbraun

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.