Skip to content
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

fix relative path expansion #4

Closed
wants to merge 1 commit into from
Closed

Conversation

catb0t
Copy link

@catb0t catb0t commented Dec 8, 2018

closes #3, for Windows as well
closes #5

before this patch

$ ./example.py 
before  False
['/usr/bin/python3', './example.py']
/usr/bin/python3: can't open file './example.py': [Errno 2] No such file or directory

# expected behaviour (with realpath) 
$ $(realpath example.py)
before  False
['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py']
before  True
after  True

after this patch

$ example.py
before  False
['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py']
before  True
after  True

@barneygale
Copy link
Owner

Thanks for your work on this. I think the bug is related the working directory for the elevated process being different (see here for pkexec). I'd guess that Windows and pkexec on Linux are affected. sudo might be affected on some Linuxes. Mac seems OK.

I need to think about this as it sys.argv[0] isn't the only thing affected - other relative paths in the command-line should also be translated, but there's no general way to identify them.

elevate/posix.py Outdated Show resolved Hide resolved
elevate/windows.py Outdated Show resolved Hide resolved
@catb0t
Copy link
Author

catb0t commented Dec 13, 2018

ok, this is WIP but pretty cool and it works in my Python 3/2.7.15+, with sudo and pkexec. the challenge is, print the first line of the first argument.

$ ./example.py README.rst 
before:  /home/cat/Sync/projects/git/elevate
before:  False
execlp   ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git/elevate', 'README.rst']
before:  /home/cat/Sync/projects/git/elevate
before:  True
after:   /home/cat/Sync/projects/git/elevate
after:   True
Elevate: Request root privileges


$ ./example.py README.rst 
before:  /home/cat/Sync/projects/git/elevate
before:  False
execlp   ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git/elevate', 'README.rst']
before:  /root
before:  True
after:   /home/cat/Sync/projects/git/elevate
after:   True
Elevate: Request root privileges


$ cd ..

$ elevate/example.py elevate/README.rst 
before:  /home/cat/Sync/projects/git
before:  False
execlp   ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git', 'elevate/README.rst']
before:  /root
before:  True
after:   /home/cat/Sync/projects/git
after:   True
Elevate: Request root privileges


$ elevate/example.py elevate/README.rst 
before:  /home/cat/Sync/projects/git
before:  False
execlp   ['/usr/bin/python3', '/home/cat/Sync/projects/git/elevate/example.py', '--_with-elevate-cwd=/home/cat/Sync/projects/git', 'elevate/README.rst']
before:  /root
before:  True
after:   /home/cat/Sync/projects/git
after:   True
['/home/cat/Sync/projects/git/elevate/example.py', 'elevate/README.rst']
Elevate: Request root privileges

this -- preserving the previous directory with a secret implementation detail -- obviated the need to translate other arguments than sys.argv[0], which is great, because that was almost a confounding problem.

now, let me go and test it / fix it up on Windows and maybe BSD or some other Linuxes. I don't have a Mac, though.

@catb0t
Copy link
Author

catb0t commented Dec 14, 2018

I can't reproduce #3 on Windows at all, especially not with the new abspath, so I don't think any further changes to that effect in Windows are needed.

@catb0t
Copy link
Author

catb0t commented Dec 14, 2018

It seems this (mostly) fixes #5 because the way the elevated process is invoked makes it very easy to prevent infinite recursion (although I could leave that for a separate PR).

(when ready for merge, I'll squash all these and get rid of all the debugging print()s and example.py)

@barneygale
Copy link
Owner

Sorry for the slow response.

This is a clever fix, and the Windows issue hadn't occurred to me.

I worry that this prevents a user calling elevate() after command-line parsing is complete, which is desirable in some cases. I'd prefer that any addition of command-line options is opt-in, something like restore_cwd=True.

As for #5, I wonder if we can't detect the failure of the UAC thing through some other means, either in the parent or child process?

@catb0t
Copy link
Author

catb0t commented Feb 20, 2019

I worry that this prevents a user calling elevate() after command-line parsing is complete, which is desirable in some cases.

I'm not sure what you mean, could you elaborate? Since elevate() re-invokes the current process anyway, it's usually desirable to call it as early as possible like setuid. But, because sys.argv is secretly changed, options to user code look exactly the same.

I'd prefer that any addition of command-line options is opt-in, something like restore_cwd=True.

Ok, I can understand that! I'll add it.

As for #5, I wonder if we can't detect the failure of the UAC thing through some other means, either in the parent or child process?

From my testing, it's completely undetectable / opaque and the only way to prevent an infinite recursion, other than sharing memory directly between the old and new processes, is by passing data to the new process invoked by elevate, through the command line (or maybe a named pipe...)

@barneygale
Copy link
Owner

Sorry for the poor explanation. I'm considering this sort of use case:

parser = argparse.ArgumentParser()
parser.add_argument('--sudo', action='store_true')
args = parser.parse_args()  # sudo'd process falls over here due to `--_` options?
if args.sudo:
    elevate.elevate()

This use case should work fine because it hasn't actually modified anything or performed any real work. I left "early" a little undefined in the README, but it basically means "as late as your program can be considered re-entrant".

From my testing, it's completely undetectable / opaque and the only way to prevent an infinite recursion, other than sharing memory directly between the old and new processes, is by passing data to the new process invoked by elevate, through the command line (or maybe a named pipe...)

How frustrating! Your approach sounds good in this case 👍

@catb0t
Copy link
Author

catb0t commented Feb 20, 2019

Ah, that is quite a significant problem I forgot about. There are only a couple options I can think of,

  • special monkey-patching of argparse, if imported, with a class that allows --_ options, except such a patch would have to exist for every argument parsing module... quite unrealisitc.

  • separate elevate's special options from the rest of sys.argv when import elevate occurs; unfortunately it requires that import elevate appear before argument parsing begins.

IMO this last point isn't a big problem considering that elevate has the responsibility of re-starting the current process and it makes sense to think that maybe it has the authority to hook its import.

@catb0t
Copy link
Author

catb0t commented Feb 20, 2019

I'd prefer that any addition of command-line options is opt-in, something like restore_cwd=True.

restore_cwd=True is already the default on posix, did you want that changed to default False?

Anyway, on that note, now there's an import hook (see the commit message) which filters and remembers the options when import elevate is used. It puts them in a module-global variable, which is not optimal but it's not like elevate is MT-safe anyway ;)

The question is, should that import hook become a public function that the user can call if they want restore_cwd to work? Or since the options are actually hidden to user code with it, there's no point allowing opt-out?

@catb0t
Copy link
Author

catb0t commented Feb 20, 2019

Actually, this is cool, there is one more solution, it's to make it into an OO interface (but the same in the backend), then the argument parsing modification is only done when the user invokes the class instead of at import time automatically.

Also, no more module global variable, that's usually a plus of an OO re-design.

Unfortunately it slightly changes elevate from being a "simple" one-function functional interface, but that's what it takes to get around these edge cases. catb0t@f48dc48

- close barneygale#3: relative path expansion
- **temporarily** fix barneygale#5 for windows
- documentation is added to the workarounds
    in elevate_util

This implementation uses an import hook, which
    may not be the cleanest solution possible.

The import hook fixes argparse and similar modules
    seeing and complaining about --_with-elevate-*
    options, if the argument parser is called before
    the re-entrant call to elevate().
@catb0t
Copy link
Author

catb0t commented Jun 18, 2019

I squashed the 11 commits on this branch and cleaned up the commit message. If you prefer the class-based OO implementation I mentioned in my last comment, over this import-hook, then I can rebase this branch on catb0t:oo before you merge.

@barneygale
Copy link
Owner

Sorry for the long delay. I'm not a fan of the import-time stuff as it feels too magic. I think the best course of action is to not add command-line arguments by default - but if users want to opt into it, we shouldn't try to hide it from them.

What do you think of 6b788dc? Only in a branch for now.

@catb0t
Copy link
Author

catb0t commented Jul 1, 2019

Sorry, I finally got around to testing your code. It's objectively superior to my implementation, including my class-based one, but I personally prefer mine.

I think it's a bit silly how basic functionality becomes opt-in, but you're the maintainer, and I think you should go with your solution.

@barneygale
Copy link
Owner

For me it's a finely-balanced thing.

  • The Windows bug is really severe, but also (as far as I can tell) fairly rare
  • The Linux "bug" (cwd not preserved) is arguably expected behaviour, though we should cater to people who feel differently. I think making absolute any path arguments strikes a good balance.

My main reason is this: I think the problem elevate attempts to solve is inherently quite finnickity, which is why this is the first PyPI-published package offering the functionality, despite a lot of SO posts on the topic over the years. It's frustrating to have a trade-off between ease-of-use and robustness, and I suspect it's a matter not only taste but also target audience: the use case I have in mind for this package is quickly-written devops "glue" scripts rather than consumer-level software. The advantage of being able to stick elevate() in your script without having to touch the command-line parser is too good to pass up, especially if someone else, long ago, used a hand-rolled or unfamiliar parser like click or getopt or something.

I appreciate the thought and effort you've put into this - I couldn't have arrived at my patch without yours, so I'm sorry to you to be closing a 6 month old PR without merging. I'm not ruling out changing the default in future if I hear more feedback. Thank you again! I promise I'm not usually this much of a diva.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Certain Windows configurations can cause an infinite recursion [Errno 2] No such file or directory
2 participants