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: |
Description (last modified by )
Attachments (3)
Change History (38)
comment:1 Changed 11 years ago by
- Cc leif added
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
- 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: ↓ 5 Changed 11 years ago by
- 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
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.
comment:6 Changed 11 years ago by
- Status changed from new to needs_review
See my monkey patch for fixing this.
See also the discussion here: http://groups.google.com/group/sage-devel/browse_thread/thread/4b50ca1df10f0ac3/242d36cdbbb194e8#242d36cdbbb194e8
comment:7 Changed 11 years ago by
- Owner changed from jason to AlexanderDreyer
comment:8 follow-up: ↓ 12 Changed 11 years ago by
Well, it's sometimes better to know who packaged an spkg (though one can easily fake this of course). ;-)
comment:9 Changed 11 years ago by
- Description modified (diff)
comment:10 Changed 11 years ago by
- Description modified (diff)
comment:11 Changed 11 years ago by
- 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: ↓ 13 Changed 11 years ago by
- 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
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
orSPKG.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, ahg 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
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
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?
comment:16 Changed 11 years ago by
- 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
- Type changed from defect to enhancement
comment:18 Changed 11 years ago by
- Description modified (diff)
comment:19 Changed 11 years ago by
- Owner changed from (none) to AlexanderDreyer
comment:20 follow-up: ↓ 22 Changed 11 years ago by
- 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
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: ↓ 23 ↓ 25 Changed 11 years ago by
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: ↓ 26 Changed 11 years ago by
- 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
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
Replying to AlexanderDreyer:
First of all, this is a patch to
sage-pkg
, notsage-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: ↓ 27 Changed 11 years ago by
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_fileon 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: ↓ 28 Changed 11 years ago by
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: ↓ 29 Changed 11 years ago by
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-likeinstall
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: ↓ 30 Changed 11 years ago by
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: ↓ 31 Changed 11 years ago by
- 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: ↓ 32 Changed 11 years ago by
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: ↓ 33 Changed 11 years ago by
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 achmod -R
onsrc/
at least (fromsage-spkg
), although this will in most cases be redundant or superfluous (e.g. ifinstall -c -m ...
is used by the spkg anyway, or all files to be copied withcp -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
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
- 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
- Resolution set to wontfix
- Status changed from positive_review to closed
What is the desired resolution?
sage-pkg
should not modify thesrc/
directory when making an SPKG.