Restore `Denied behaviour of PATH resolution - #4265
Merged
Merged
Conversation
Member
Author
|
Excellent, our old friend the |
dra27
force-pushed
the
break-cygwin-again
branch
6 times, most recently
from
July 13, 2020 13:28
614344d to
0921640
Compare
dra27
marked this pull request as ready for review
July 13, 2020 13:42
Member
|
Wow! Impressive detective work, and I agree with your recommendations about testing |
dra27
force-pushed
the
break-cygwin-again
branch
from
July 17, 2020 08:22
0921640 to
ac3474d
Compare
When resolving a command in PATH, `Not_found should be returned only if the command was not in all the directories listed in PATH. If the command was found in at least one, but none of the commands found were executable, then `Denied should be returned instead.
A principal does not get the union of user, group and other permissions - you only ever get one part of the mask.
Permissions checking is based on effective UID. It is undefined whether getgroups includes the effective GID, so include it in the list explicitly.
libacl is only mandatory for Cygwin and must be explicitly selected for other platforms. For now, expressly binding to libacl in order to have acl_get_perm, rather than adding general ACL support (see libarchive for the full horrendous details of how to do this portably otherwise).
Need 3.1.7 when it's released.
rjbou
force-pushed
the
break-cygwin-again
branch
from
July 21, 2020 10:37
ac3474d to
61a0d62
Compare
Collaborator
|
Rebased & fix upgrade job (no hash needed for opam 1.2.2 init) |
Member
Author
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on from #4072, which alas is evidence that absence of evidence is not evidence of absence 🙂
This PR alters
OpamSystem.t_resolve_commandso that`Not_foundis only returned ifnamewas not present in of the directories ofPATH. Ifnameis present in at least one directory, but all the matches fail the permissions check, then`Deniedis now returned instead, which more accurately mirrors the previous behaviour (and is also consistent with the shell's behaviour).This is a draft PR, as sadly this re-opens my need to fix the Cygwin's permission check...While working on this, I also noticed that
check_permswas not using the correct set of uids/gids - it now correctly uses the EUID and also ensures that EGID is included in the list of groups considered (sincegetgroupsis not required to return the EGID).The fix to the behaviour restores the breakage which we'd seen on Cygwin CI since late September last year. I chased down what was broken in January, but erroneously put fixing it properly on the backburner as CI started working when #4072 was merged.
The history is mildly interesting, and yields another cautionary tale for our testing:
"cmd": permission denied.on the Cygwin32 build during initial switch creation. This was coming from the command inOpamSysPoll.os_version_lazyfailing the permissions check inOpamSystem.t_resolve_command. As we didn't think we'd changed anything, I initially assumed that Cygwin had changed the way the ACL handling was working, but that was incorrect (it hasn't changed since at least Cygwin 1.7, which is a very long time ago). It turns out that 2 packages from issuu/ocaml-protoc-plugin at 0.9 opam-repository#14908 innocently created the first opam package in opam-repository to useos-versionin anavailablefield which means that determining the set of available packages will now always force that variable. This was merged on 26 Sep 2019, and you can then see the error appearing in our AppVeyor logs for all PRs and branches after this time. One can test with the previous SHAopam init git+https://github.com/ocaml/opam-repository.git#3303f6and opam built at that time. However, depexts integration #3975 then cemented the issue further - there have been several packages (correctly) usingos-versionindepextso in fact you can only get the pre 26 Sep 2019 behaviour by runningopam init git+https://github.com/ocaml/opam-repository.git#3303f6 --no-depexts!OPAM_REPO_SHAdefined for both Travis and AppVeyor and allopam initinstructions now use the opam-repository Git repo. The only downside is that it means Travis and AppVeyor no longer test the HTML backend - if this is a concern, then I guess we could mirror opam-repository in order to have anindex.tar.gzat our own SHA?Therefore:
Is entirely expected. The execute permission cannot be determined from
stat, therefore, but only by checking the ACL.--with-libaclwhich requires libacl-like support rather than--enable-acl, which would be the more complete "do whatever is necessary to get ACL support on this platform" (the horrific detail of how you get a cross-platform version of the so-niche-that-they-didn't-include-it-in-the-draft functionacl_get_permcan be seen in libarchive's sources). On Cygwin,libaclsupport is required (i.e.--with-libaclis the default), on all other platforms it must be explicitly selected. You can explicitly disable libacl support on Cygwin with--without-libaclbut your opam won't work properly. The support appears to work OK on Ubuntu (where it correctly detects the need for-lacl) and Cygwin (where it correctly detects that no linker options are required).The final entertainment is that while Cygwin's libacl works correctly on 64-bit, it turns out that there's a bug affecting two of the functions on 32-bit Cygwin! My fix to that bug has been merged upstream but, until Cygwin 3.1.7 is released, AppVeyor is switched to use a developer snapshot of the Cygwin DLL (as it happens, this fix is the only new thing in that snapshot), I'll keep an eye on Cygwin releases and update AppVeyor after the next Cygwin release. If used on Cygwin 3.1.6 and earlier, the ACL simply fails to see the execute permission and you get the "Permission denied" error, as before.